Closed michael-grunder closed 5 months ago
Hi @michael-grunder,
Thanks for the detailed issue, and the reproducer! I believe I understand what was going on, for each entry that was modified, the function would change the probes number, but failed to restore it if we failed to allocate memory. It should be fixed with 872522abff84afceb5af3835bd8c3875de690674 Can you confirm it is fixed for you ?
Thanks!
Will do when I'm back at my desk. Thanks for such a quick reply!
https://github.com/concurrencykit/ck/commit/872522abff84afceb5af3835bd8c3875de690674 fixes the issue for me locally. Thank you so much!
Hi everyone :wave:,
I think I found another OOM edge case.
fail_to_allocate: yes, read_mostly: no, limit: 0
--- Adding 9 elements ---
[0] Attmpting to add 0x1
[1] Attmpting to add 0x2
[2] Attmpting to add 0x3
[3] Attmpting to add 0x4
[4] Attmpting to add 0x5
Failing to allocate 4519 bytes
[5] Attmpting to add 0x6
Failing to allocate 4519 bytes
[6] Attmpting to add 0x9
Failing to allocate 4519 bytes
[7] Attmpting to add 0xa
Failing to allocate 4519 bytes
[8] Attmpting to add 0xb
Failing to allocate 4519 bytes
Failed to insert: 0xb
--- Removing 8 elements ---
[0] Attempting to remove 0xa
OK 0xa == 0xa
[1] Attempting to remove 0x3
ERR Failed to remove 0x3 (not found)
This time I'm using CK_RHS_MODE_DIRECT
just to make sure the issue didn't have to do with the map using the high bits for optimizations.
One interesting thing is that the ck_rhs_set
operation (that fails because there are no more free buckets), increases a bucket's probes
to 15
which seems wrong, although I only partially understand the probing logic.
Before the final ck_rhs_set
that fails:
map buckets:
0) probes: 3, wanted: 2, probe_bound: 0, in_rh: T, entry: 0xa
1) probes: 3, wanted: 3, probe_bound: 3, in_rh: F, entry: 0x3
2) probes: 2, wanted: 3, probe_bound: 5, in_rh: T, entry: 0x9
3) probes: 3, wanted: 3, probe_bound: 3, in_rh: F, entry: 0x1
4) probes: 1, wanted: 0, probe_bound: 1, in_rh: F, entry: 0x4
5) probes: 5, wanted: 1, probe_bound: 2, in_rh: F, entry: 0x2
6) probes: 2, wanted: 1, probe_bound: 2, in_rh: F, entry: 0x5
7) probes: 2, wanted: 0, probe_bound: 0, in_rh: F, entry: 0x6
After the failed insertion:
map buckets:
0) probes: 3, wanted: 2, probe_bound: 0, in_rh: T, entry: 0xa
1) probes: 15, wanted: 3, probe_bound: 6, in_rh: F, entry: 0x3
2) probes: 2, wanted: 3, probe_bound: 15, in_rh: T, entry: 0x9
3) probes: 5, wanted: 3, probe_bound: 5, in_rh: F, entry: 0x1
4) probes: 3, wanted: 0, probe_bound: 5, in_rh: F, entry: 0x4
5) probes: 3, wanted: 1, probe_bound: 3, in_rh: F, entry: 0x2
6) probes: 2, wanted: 1, probe_bound: 3, in_rh: F, entry: 0x5
7) probes: 1, wanted: 0, probe_bound: 0, in_rh: F, entry: 0x6
After the first ck_rhs_remove
and its backward shift delete, we end up with this:
0) probes: 0, wanted: 0, probe_bound: 0, in_rh: T, entry: NULL
1) probes: 1, wanted: 2, probe_bound: 6, in_rh: F, entry: 0x9
2) probes: 1, wanted: 1, probe_bound: 2, in_rh: T, entry: 0x3
3) probes: 5, wanted: 1, probe_bound: 5, in_rh: F, entry: 0x1
4) probes: 3, wanted: 0, probe_bound: 5, in_rh: F, entry: 0x4
5) probes: 3, wanted: 1, probe_bound: 3, in_rh: F, entry: 0x2
6) probes: 2, wanted: 1, probe_bound: 3, in_rh: F, entry: 0x5
7) probes: 1, wanted: 0, probe_bound: 0, in_rh: F, entry: 0x6
The second deletion 0x3
maps to bucket 3:
3) probes: 5, wanted: 1, probe_bound: 5, in_rh: F, entry: 0x1
The probe depth is too low and it is not found.
If in gdb
I manually reset the probes
, wanted
, and probe_bound
to the values before the failed ninth insertion, the program completes without issues and everything is found.
Let me know if you have any questions or if I am confused about something.
Cheers
Hey @michael-grunder !
Thanks for reporting this one too! And hopefully there won't be much more to come. I believe I understood what was happening, and fixed it with commit 53d8e34bbb9a0c569ce7daa091d89e729864eb9e, but honestly this is code I wrote quite a long time ago, and getting the logic is hard for me too, so please let me know if it gets you more trouble.
I can confirm that 53d8e34 fixes the second edge case for me.
Thanks again for taking a look so quickly!
I encountered what might be an edge case in
ck_rhs_t
causing inconsistency in the map.The issue has nothing to do with concurrency so if I'm doing something obviously wrong it should be easy to see.
What I'm doing is creating a
CK_RHS_MODE_OBJECT
map with a capacity of 8 (which appears to be whatck_rhs_t
actually picks). I allow the allocation of the initial map to succeed, but then fail every additional call to our allocator, whenck_rhs_t
attempts to grow.ck_rhs_t
stores 8 elements, failing when we try to store the ninth. I then iterate over the elements in the map deleting them. This appears to leadck_rhs_t
to an inconsistent state, where it fails to find the final element.I've created a repo that demonstrates the behavior
The repo also includes a GDB pretty printer to dump the internal state of the
ck_rhs_t
.One thing that did catch my eye is that on the failed
ck_rhs_set
the internal state gets modified where everyin_rh
->true
becomesin_rh
->false
https://github.com/concurrencykit/ck/blob/471558111ff7c376450724945665cbf5033534a5/src/ck_rhs.c#L939-L944Before:
And after:
I didn't see anything in the documentation about a failure in
ck_rhs_set
meaning the map can't be further used but perhaps I missed it. Also, usingCK_RHS_MODE_READ_MOSTLY
fixes the problem as it probes more deeply.You can run the demo program with different options to demonstrate slightly different behavior.
The default "hash" function effectively hashes the keys to themselves as an integer just to make it easier to map them back and forth. You can pass
--djb3
to use a real hash function.I don't think I'm missing anything obvious, but it's certainly possible :smile:
Thanks!