support inline skiplist

[jayzhan211]

No description provided.

[codecov]

Codecov Report

Merging #85 into master will increase coverage by 0.15%.
The diff coverage is 80.56%.

@@            Coverage Diff             @@
##           master      #85      +/-   ##
==========================================
+ Coverage   74.86%   75.01%   +0.15%     
==========================================
  Files          38       39       +1     
  Lines        9422    10028     +606     
  Branches     1899     2073     +174     
==========================================
+ Hits         7054     7523     +469     
+ Misses        767      723      -44     
- Partials     1601     1782     +181     

Impacted Files	Coverage Δ
src/mem/mod.rs	84.24% <ø> (+1.50%)	⬆️
src/mem/inlineskiplist.rs	79.20% <79.20%> (ø)
src/mem/arena.rs	80.00% <88.09%> (+1.48%)	⬆️
src/mem/skiplist.rs	79.37% <88.23%> (-1.89%)	⬇️
src/util/slice.rs	0.00% <0.00%> (-28.85%)	⬇️
src/table_cache.rs	83.33% <0.00%> (-1.67%)	⬇️
src/options.rs	84.04% <0.00%> (-1.52%)	⬇️
src/batch.rs	81.14% <0.00%> (-1.35%)	⬇️
src/record/reader.rs	76.66% <0.00%> (-1.26%)	⬇️
... and 25 more

---

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 9bd9ee4...7a2f6ab. Read the comment docs.

[jayzhan211]

@Fullstop000 Maybe you can open a new PR for an offset-based arena and its tests. Because it seems to affect merged files as well.

[Fullstop000]

@accelsao Concurrent tests are failed due to invalid memory ref. And I've tested them with gdb and Valgrind but didn't found the cause. The AddressSanitizer is giving some errors but I have no idea how to address the related codes. PTAL if you have time :)

[Fullstop000]

@accelsao I think I might find the root cause of UB:
In previous implementation, when allocating a Node, we init the next_nodes by calling ptr::write_bytes

ptr::write_bytes(&mut (*p).next_nodes, 0, height);

which writes height * size_of::<[AtomicPtr<Node>; MAX_HEIGHT]>() bytes. Just a huge mistake 😂 especially in concurrent insertion.

Instead, we should use:

ptr::write_bytes((&mut (*p)).next_nodes.as_mut_ptr(), 0, height);

which correctly writes height * size_of::<AtomicPtr<Node>>() bytes.

Is it OK to force-push my fixed patch to this PR？ It seems you've added two commits.

[jayzhan211]

@accelsao I think I might find the root cause of UB:
In previous implementation, when allocating a Node, we init the next_nodes by calling ptr::write_bytes

ptr::write_bytes(&mut (*p).next_nodes, 0, height);

which writes height * size_of::<[AtomicPtr<Node>; MAX_HEIGHT]>() bytes. Just a huge mistake 😂.

Instead, we should use:

ptr::write_bytes((&mut (*p)).next_nodes.as_mut_ptr(), 0, height);

which correctly writes height * size_of::<AtomicPtr<Node>>() bytes.

Cool !

Is it OK to force-push my fixed patch to this PR？ It seems you've added two commits.

OK

[jayzhan211]

how do you find ub, from test?

[jayzhan211]

current failures:

    db::tests::test_get_from_immutable_layer
    db::tests::test_minor_compactions_happend
    db::tests::test_recover_with_large_log

It seems they are independent of this PR.

[Fullstop000]

how do you find ub, from test?

AddressSanitizer provides some clues about invalid memory ref when dropping Node so I checked allocated bytes and found the size of next_nodes is not as expected

current failures:

    db::tests::test_get_from_immutable_layer
    db::tests::test_minor_compactions_happend
    db::tests::test_recover_with_large_log

It seems they are independent of this PR.

As we don't alloc memory in the arena for key content at all (now managed by Bytes itself), the result memory_used is not consistent as before (much smaller than before) so that all the tests minor compaction involved (it checks the memory_used) will fail.

[Fullstop000]

Currently, memory_used is accumulated with the content length of the key even if they are not part of arena allocated bytes to stay consistent with previous behavior.
@accelsao PTAL