ck_rhs: Fix robin-hood probe depth calculations and a ck_rhs_fas edge case. - #282
Conversation
Allocation failure surfaced an edge case where robin-hood probing limit could become inaccurate. The symptom we were seeing was the following: ```c writeLock(); k = ck_rhs_get(hs, h, k); d = ck_rhs_remove(hs, h, k); assert(d == k); // fails because `d == NULL` ``` Standalone reproducer: https://gist.github.com/michael-grunder/516e6ceb0b93b5f02f7aaac259f9e8d9 The symptom was more likely in low memory conditions causing allocation failures but could occur without them. The following three conditions could lead to the error: 1. A full Robin Hood relocation history switched to a probe mode that restarted the search instead of continuing the current relocation. 2. A successful Robin Hood relocation could leave its original slot marked `in_rh` erroneously. 3. Backward-shift deletion reconstructed a candidate's home bucket with a probe count that was one too small. This could lower a probe bound while a live entry remained beyond it. A new `probe_bound.c` regression test is introduced that attempts to surface all of the above edge cases. Because some of the tests need to peek into private `ck_rhs_map` struct members it includes the actual `src/ck_rhs.c` file not just the header. This commit also includes a small refactor of logic after we are certain the map just grew on us. Previously in these cases the code was setting `in_rh = false` which was not required since the larger reallocated map always has these entries initialized to false.
The ck_rhs_fas function can erroneously return success without actually replacing the entry when Robin Hood relocation grows the table. After growth, subsequent probing continued using the old, newly retired map, causing the replacement to be lost. The fix is simple. Just update the map pointer when we jump to `restart`. Reproducer: https://gist.github.com/michael-grunder/b0d396001843b46aaa89b58fefd99819
|
Hey @michael-grunder, thanks for this! |
Yeah you've got it. What we were seeing in production, especially under allocation failures keeping the table denser than ideal was /* Invariant: k is in the map but `rhs` will not probe far enough to find it */
d = ck_rhs_remove(hs, h, k);
assert(d == k); // fails because `d == NULL`Whether this is the optimal solution I'm not sure. Mostly the fix was one edge case that could leave an |
|
That makes to me, even if it is not optimal (and honestly I don't see how to do this better either, except for rewriting the whole thing), at least it is correct, so I'll merge it, we will still be able to make it better later. |
2 participants
The two commits have detailed messages but the top-level changes are as follows:
ck_rhs_fasreturns success but actually doesn't replace the key. This can happen if the robin-hood insert logic grows the table.I don't think I'm doing anything obviously wrong, but this is a very advanced hash set. In addition to the two regressions I did quite a lot of fuzzing of the map and could not find any introduced flaws.
The existing serial benchmark is within around 1% of performance on my x86-64 xeon so close to noise although it may be slightly slower due to the correctness fixes.
I also created a more granular benchmark script:
https://gist.github.com/michael-grunder/e3819c48617f130be8e2c75beee75ed8
The numbers from master and the feature branch are comparable.