I have been working on a RedBlack Tree implementation in C (Red Black Tree Node Insertion Overwrites Previously Added Node) and have run into an issue where after a large number of deletions (~1000+ to ~2000+), it gets stuck in an infinite loop.
RB delete code:
typedef struct TreeNode TreeNode;
struct TreeNode {
TreeNode *parent;
TreeNode *child[2];
COLOR color;
U64 blockSize; // 'key'
void *data;
};
typedef struct Tree Tree;
struct Tree {
TreeNode *root;
TreeNode *nil;
};
...
void transplant(Tree *tree, TreeNode *u, TreeNode *v) {
if (u->parent == tree->nil) {
tree->root = v;
} else if (u == u->parent->left) {
u->parent->left = v;
} else {
u->parent->right = v;
}
v->parent = u->parent;
}
void delete_fix(Tree *tree, TreeNode *x) {
TreeNode *w = tree->nil;
while (x != tree->root && x->color == BLACK) {
if (x == x->parent->left) {
w = x->parent->right;
if (w->color == RED) {
w->color = BLACK;
x->parent->color = RED;
left_rotate(tree, x->parent);
w = x->parent->right;
}
if (w->left->color == BLACK && w->right->color == BLACK) {
w->color = RED;
x = x->parent;
} else {
if (w->right->color == BLACK) {
w->left->color = BLACK;
w->color = BLACK;
right_rotate(tree, w);
w = x->parent->right;
}
w->color = x->parent->color;
x->parent->color = BLACK;
w->right->color = BLACK;
left_rotate(tree, x->parent);
x = tree->root;
}
} else {
w = x->parent->left;
if (w->color == RED) {
w->color = BLACK;
x->parent->color = RED;
right_rotate(tree, x->parent);
w = x->parent->left;
}
if (w->right->color == BLACK && w->left->color == BLACK) {
w->color = RED;
x = x->parent;
} else {
if (w->left->color == BLACK) {
w->right->color = BLACK;
w->color = BLACK;
left_rotate(tree, w);
w = x->parent->left;
}
w->color = x->parent->color;
x->parent->color = BLACK;
w->left->color = BLACK;
right_rotate(tree, x->parent);
x = tree->root;
}
}
}
x->color = BLACK;
}
void delete_node(Tree *tree, TreeNode *z) {
if (tree != NIL && z != tree->nil && z != tree->nil) {
TreeNode *x = tree->nil, *y = z;
COLOR yOgColor = y->color;
if (z->left == tree->nil) {
x = z->right;
transplant(tree, z, z->right);
} else if (z->right == tree->nil) {
x = z->left;
transplant(tree, z, z->left);
} else {
y = minimum(tree, z->right);
yOgColor = y->color;
x = y->right;
if (y->parent == z) {
x->parent = y;
} else {
transplant(tree, y, y->right);
y->right = z->right;
y->right->parent = y;
}
transplant(tree, z, y);
y->left = z->left;
y->left->parent = y;
y->color = z->color;
}
if (yOgColor == BLACK) {
delete_fix(tree, x);
}
}
}
TreeNode *minimum(Tree *t, TreeNode *x) {
while (x->left != t->nil) {
x = x->left;
}
return (x);
}
I have narrowed down where it hits an infinite loop, but the main culprit must be in the rest of the delete_node implmentation:
TreeNode *minimum(Tree *t, TreeNode *x) {
while (x->left != t->nil) {
x = x->left;
}
return (x);
}
Driver Code
U64 myrandom(U64 range) {
U64 num;
num = rand() % range;
return num;
}
void print_inorder(FILE *file, Tree *t, TreeNode *n) {
if (n != t->nil) {
print_inorder(file, t, n->left);
fprintf(file, "%llu \n", n->blockSize);
print_inorder(file, t, n->right);
}
}
int main(void) {
srand(time(NULL));
printf("Hello Tree\n");
Tree tree = {0};
TreeNode nil = {0};
nil.parent = NIL;
nil.left = &nil;
nil.right = &nil;
nil.color = BLACK;
nil.blockSize = 0;
tree.nil = tree.root = &nil;
Tree *ptr = &tree;
LARGE_INTEGER frequency;
LARGE_INTEGER start;
LARGE_INTEGER end;
double interval;
QueryPerformanceFrequency(&frequency);
QueryPerformanceCounter(&start);
TreeNode nodes[20000];
for (int i = 0; i < 20000; i++) {
TreeNode n = {0};
n.parent = NIL;
n.left = NIL;
n.right = NIL;
n.color = BLACK;
n.blockSize = myrandom(UINT_FAST64_MAX);
nodes[i] = n;
}
QueryPerformanceCounter(&start);
for (int i = 0; i < 20000; i++) {
insert_node(ptr, &nodes[i]);
}
QueryPerformanceCounter(&end);
interval = (double) (end.QuadPart - start.QuadPart) / frequency.QuadPart;
FILE *preDelFilePtr = fopen("rb_test_pre_delete.txt", "wb");
fprintf(preDelFilePtr, "************************\n");
fprintf(preDelFilePtr, "* pre delete *\n");
fprintf(preDelFilePtr, "************************\n");
fprintf(preDelFilePtr, "* time spent: %f *\n", interval);
fprintf(preDelFilePtr, "************************\n");
print_inorder(preDelFilePtr, ptr, ptr->root);
frequency.QuadPart = 0;
start.QuadPart = 0;
end.QuadPart = 0;
interval = 0;
QueryPerformanceCounter(&start);
for (int i = 0; i <= 10000; i++) {
U64 removeIndex = myrandom(20000);
if (&nodes[removeIndex] != ptr->nil)
{
delete_node(ptr, &nodes[removeIndex]);
}
}
QueryPerformanceCounter(&end);
interval = (double) (end.QuadPart - start.QuadPart) / frequency.QuadPart;
FILE *postDelFilePtr = fopen("rb_test_post_delete.txt", "wb");
fprintf(postDelFilePtr, "************************\n");
fprintf(postDelFilePtr, "* post delete *\n");
fprintf(postDelFilePtr, "************************\n");
fprintf(postDelFilePtr, "* time spent: %f *\n", interval);
fprintf(postDelFilePtr, "************************\n");
print_inorder(postDelFilePtr, ptr, ptr->root);
fclose(preDelFilePtr);
fclose(postDelFilePtr);
return 0;
}
So far, I have tried adding some checks at the beginning of the minimum function as well, but that didn't solve the issue either.
Any help or comments would be appreciated!
UPDATE:
Below is the updated full source which now uses NULL instead of tree->nil to represent a leaf node.
main.c
#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <time.h>
#include <windows.h>
#include "rb.h"
U64 myrandom(U64 range) {
U64 num;
num = rand() % range;
return num;
}
void print_inorder(FILE *file, Tree *t, TreeNode *n) {
if (n != NULL) {
print_inorder(file, t, n->left);
fprintf(file, "%llu \n", n->blockSize);
print_inorder(file, t, n->right);
}
}
int main(void) {
srand(time(NULL));
printf("Hello Tree\n");
Tree tree = {0};
Tree *ptr = &tree;
TreeNode nodes[20000];
for (int i = 0; i < 20000; i++) {
TreeNode n = {0};
n.parent = NULL;
n.left = NULL;
n.right = NULL;
n.color = BLACK;
n.blockSize = myrandom(UINT_FAST64_MAX);
nodes[i] = n;
}
FILE *preDelFilePtr = fopen("rb_test_pre_delete.txt", "wb");
fprintf(preDelFilePtr, "************************\n");
fprintf(preDelFilePtr, "* pre delete *\n");
print_inorder(preDelFilePtr, ptr, ptr->root);
FILE *postDelFilePtr = fopen("rb_test_post_delete.txt", "wb");
fprintf(postDelFilePtr, "************************\n");
fprintf(postDelFilePtr, "* post delete *\n");
print_inorder(postDelFilePtr, ptr, ptr->root);
fclose(preDelFilePtr);
fclose(postDelFilePtr);
return 0;
}
rb.h
#ifndef RB_H
#define RB_H
#include <inttypes.h>
typedef enum COLOR { BLACK, RED } COLOR;
#ifndef U64_TYPE
#define U64_TYPE
typedef uint_fast64_t U64;
#endif
typedef struct TreeNode TreeNode;
struct TreeNode {
TreeNode *parent;
TreeNode *child[2];
COLOR color;
U64 blockSize; // 'key'
void *data;
};
typedef struct Tree Tree;
struct Tree {
TreeNode *root;
};
#define LEFT 0
#define RIGHT 1
#define left child[LEFT]
#define right child[RIGHT]
void left_rotate(Tree *tree, TreeNode *node);
void right_rotate(Tree *tree, TreeNode *node);
void insert_fix(Tree *tree, TreeNode *node);
void insert_node(Tree *tree, TreeNode *toInsert);
void transplant(Tree *tree, TreeNode *n1, TreeNode *n2);
void delete_fix(Tree *tree, TreeNode *node);
void delete_node(Tree *tree, TreeNode *node);
TreeNode *maximum(TreeNode *node);
TreeNode *minimum(TreeNode *node);
void left_rotate(Tree *tree, TreeNode *node) {
if (tree != NULL && node != NULL) {
TreeNode *rightChild = node->right;
node->right = rightChild->left;
if (rightChild->left != NULL) {
rightChild->left->parent = node;
}
rightChild->parent = node->parent;
if (node->parent == NULL) {
tree->root = rightChild;
} else if (node == node->parent->left) {
node->parent->left = rightChild;
} else {
node->parent->right = rightChild;
}
rightChild->left = node;
node->parent = rightChild;
}
}
void right_rotate(Tree *tree, TreeNode *node) {
if (tree != NULL && node != NULL) {
TreeNode *leftChild = node->left;
node->left = leftChild->right;
if (leftChild->right != NULL) {
leftChild->right->parent = node;
}
leftChild->parent = node->parent;
if (node->parent == NULL) {
tree->root = leftChild;
} else if (node == node->parent->right) {
node->parent->right = leftChild;
} else {
node->parent->left = leftChild;
}
leftChild->right = node;
node->parent = leftChild;
}
}
void insert_fix(Tree *tree, TreeNode *z) {
// iterate until z is not the root and z's parent color is red
if (z->parent != NULL)
{
while (z != tree->root && z->parent != NULL && z->parent->color == RED) {
if (z->parent->parent != NULL)
{
if (z->parent == z->parent->parent->left &&
z->parent->parent->right != NULL) {
TreeNode *y = z->parent->parent->right;
{
if (y->color == RED) {
z->parent->color = BLACK;
y->color = BLACK;
z->parent->parent->color = RED;
z = z->parent->parent;
} else {
if (z == z->parent->right) {
z = z->parent;
left_rotate(tree, z);
}
z->parent->color = BLACK;
z->parent->color = RED;
right_rotate(tree, z->parent->parent);
}
}
} else if (z->parent == z->parent->parent->right &&
z->parent->parent->left != NULL) {
TreeNode *y = z->parent->parent->left;
if (y->color == RED) {
z->parent->color = BLACK;
y->color = BLACK;
z->parent->parent->color = RED;
z = z->parent->parent;
} else {
if (z == z->parent->left) {
z = z->parent;
right_rotate(tree, z);
}
z->parent->color = BLACK;
z->parent->color = RED;
left_rotate(tree, z->parent->parent);
}
} else {
break;
}
} else {
break;
}
}
tree->root->color = BLACK;
}// keep root always black
}
void insert_node(Tree *tree, TreeNode *z) {
if (tree != NULL) {
TreeNode *y = NULL;
TreeNode *x = tree->root;
while (x != NULL) {
y = x;
if (z->blockSize < x->blockSize) {
x = x->left;
} else {
x = x->right;
}
}
z->parent = y;
if (y == NULL) {
tree->root = z;
} else if (z->blockSize < y->blockSize) {
y->left = z;
} else {
y->right = z;
}
z->left = NULL;
z->right = NULL;
z->color = RED;
insert_fix(tree, z);
}
}
void transplant(Tree *tree, TreeNode *u, TreeNode *v) {
if (v != NULL)
{
if (u->parent == NULL) {
tree->root = v;
} else if (u == u->parent->left) {
u->parent->left = v;
} else {
u->parent->right = v;
}
v->parent = u->parent;
}
}
void delete_fix(Tree *tree, TreeNode *x) {
TreeNode *w = NULL;
if (x != NULL)
{
while (x != tree->root && x->color == BLACK) {
if (x->parent != NULL)
{
if (x == x->parent->left) {
w = x->parent->right;
if (w->color == RED) {
w->color = BLACK;
x->parent->color = RED;
left_rotate(tree, x->parent);
w = x->parent->right;
}
if (w->left != NULL && w->right != NULL)
{
if (w->left->color == BLACK && w->right->color == BLACK) {
w->color = RED;
x = x->parent;
} else {
if (w->right != NULL)
{
if (w->right->color == BLACK) {
w->left->color = BLACK;
w->color = BLACK;
right_rotate(tree, w);
w = x->parent->right;
}
}
w->color = x->parent->color;
x->parent->color = BLACK;
w->right->color = BLACK;
left_rotate(tree, x->parent);
x = tree->root;
}
}
} else {
w = x->parent->left;
if (w->color == RED) {
w->color = BLACK;
x->parent->color = RED;
right_rotate(tree, x->parent);
w = x->parent->left;
}
if (w->left != NULL && w->right != NULL)
{
if (w->right->color == BLACK && w->left->color == BLACK) {
w->color = RED;
x = x->parent;
} else {
if (w->left->color == BLACK) {
w->right->color = BLACK;
w->color = BLACK;
left_rotate(tree, w);
w = x->parent->left;
}
w->color = x->parent->color;
x->parent->color = BLACK;
w->left->color = BLACK;
right_rotate(tree, x->parent);
x = tree->root;
}
}
}
} else {
break;
}
}
x->color = BLACK;
}
}
void delete_node(Tree *tree, TreeNode *z) {
if (tree != NULL && z != NULL && z != NULL) {
TreeNode *x = NULL, *y = z;
COLOR yOgColor = y->color;
if (z->left == NULL) {
x = z->right;
transplant(tree, z, z->right);
} else if (z->right == NULL) {
x = z->left;
transplant(tree, z, z->left);
} else {
y = minimum(z->right);
yOgColor = y->color;
x = y->right;
if (x != NULL)
{
if (x->parent != NULL && y->parent == z) {
x->parent = y;
} else {
transplant(tree, y, y->right);
y->right = z->right;
y->right->parent = y;
}
}
transplant(tree, z, y);
y->left = z->left;
y->left->parent = y;
y->color = z->color;
}
if (yOgColor == BLACK) {
delete_fix(tree, x);
}
}
}
TreeNode *maximum(TreeNode *x) {
while (x->right != NULL) {
x = x->right;
}
return (x);
}
TreeNode *minimum(TreeNode *x) {
while (x->left != NULL) {
x = x->left;
}
return (x);
}
#endif //RB_H
I found that there are errors in the logic for insertion, so this needs to be solved first. But instead of trying to find all the mistakes in the insertion and deletion logic, I thought to rewrite it from scratch, also taking a slightly different approach on the following:
Don't create nodes in the main program, but have the tree-related functions take values instead of nodes, and have them create, find & dispose nodes as needed.
Benefit from the
childarray to use the same code for left/right mirror scenarios.Add a function to print the tree with indentation, so there is some visual feel of the tree structure
Add a function to verify that the tree is a valid red-black tree, including checks of:
Here is the code: