Extend radix_tree tests and disable operator-- (for MtMode)

[lukaszstolarczuk]

• disable operator-- when MtMode is enabled
• add concurrent decrement test that is failing at the moment
• extend existing radix_tree tests
• cleanup

random fails in try_decrement()

radix_concurrent_erase: /home/luke/repos/luke/libpmemobj-cpp/include/libpmemobj++/experimental/radix_tree.hpp:3251: bool pmem::obj::experimental::radix_tree<Key, Value, BytesView, MtMode>::radix_tree_iterator<IsConst>::try_decrement()
[with bool IsConst = false; Key = unsigned int; Value = pmem::obj::p<unsigned int>; BytesView = pmem::obj::experimental::bytes_view<unsigned int>; bool MtMode = true]:
Assertion `parent_ptr' failed.

radix_concurrent_erase: /home/luke/repos/luke/libpmemobj-cpp/include/libpmemobj++/experimental/radix_tree.hpp:3240: bool pmem::obj::experimental::radix_tree<Key, Value, BytesView, MtMode>::radix_tree_iterator<IsConst>::try_decrement()
[with bool IsConst = false; Key = unsigned int; Value = pmem::obj::p<unsigned int>; BytesView = pmem::obj::experimental::bytes_view<unsigned int>; bool MtMode = true]:
Assertion `r' failed.

---

This change is 

[igchor]

Reviewed 7 of 9 files at r1.
Reviewable status: 7 of 9 files reviewed, 2 unresolved discussions (waiting on @lukaszstolarczuk)

---

include/libpmemobj++/experimental/radix_tree.hpp, line 3155 at r1 (raw file):

/*
 * If MtMode == true it's not safe to use this operator (iterator may end up
 * invalid if concurrent erase happen).

Actually, the operator++ should work just fine, there's a problem only with operator--.

---

tests/radix_tree/radix_concurrent_erase.cpp, line 161 at r1 (raw file):

// 								i,
// 								value_repeats));
// 					++it;

The assertion fires here beacuse you are going out of bounds (call ++it on iterator which is already end()). This loops iterates for INITIAL_ELEMENTS times, but the actual number of element in the tree can be less due to the concurrent erases. If you fix this, this test should pass I belive.

[lukaszstolarczuk]

Reviewable status: 4 of 9 files reviewed, 2 unresolved discussions (waiting on @igchor)

---

include/libpmemobj++/experimental/radix_tree.hpp, line 3155 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Actually, the operator++ should work just fine, there's a problem only with operator--.

Done.

---

tests/radix_tree/radix_concurrent_erase.cpp, line 161 at r1 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

The assertion fires here beacuse you are going out of bounds (call ++it on iterator which is already end()). This loops iterates for INITIAL_ELEMENTS times, but the actual number of element in the tree can be less due to the concurrent erases. If you fix this, this test should pass I belive.

Done.

[codecov]

Codecov Report

Merging #1156 (75d04ac) into master (0ddba32) will increase coverage by 0.02%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master    #1156      +/-   ##
==========================================
+ Coverage   94.05%   94.08%   +0.02%     
==========================================
  Files          52       52              
  Lines        5163     5154       -9     
==========================================
- Hits         4856     4849       -7     
+ Misses        307      305       -2     

Flag	Coverage Δ
tests_clang_debug_cpp17	93.65% <ø> (+0.10%)	⬆️
tests_gcc_debug	90.56% <ø> (-0.24%)	⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

Impacted Files	Coverage Δ
include/libpmemobj++/experimental/radix_tree.hpp	98.46% <ø> (+0.14%)	⬆️
...libpmemobj++/detail/enumerable_thread_specific.hpp	98.91% <0.00%> (-1.09%)	⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp	94.35% <0.00%> (+0.15%)	⬆️
include/libpmemobj++/transaction.hpp	82.35% <0.00%> (+0.84%)	⬆️

---

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 0ddba32...75d04ac. Read the comment docs.

[igchor]

Reviewable status: 4 of 9 files reviewed, 2 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

---

tests/radix_tree/radix_concurrent_erase.cpp, line 104 at r3 (raw file):

//								i,
//								value_repeats));
//					--it;

I think that this test has the same problem as the one for operator++ (that is, if we fix operator--, this test will still fail).

[lukaszstolarczuk]

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @igchor)

---

tests/radix_tree/radix_concurrent_erase.cpp, line 104 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

I think that this test has the same problem as the one for operator++ (that is, if we fix operator--, this test will still fail).

right now it.. works - I run this in the loop and it didn't fail... so I guess, something is wrong...?

[igchor]

Reviewable status: 0 of 9 files reviewed, 2 unresolved discussions (waiting on @igchor and @lukaszstolarczuk)

---

tests/radix_tree/radix_concurrent_erase.cpp, line 104 at r3 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

right now it.. works - I run this in the loop and it didn't fail... so I guess, something is wrong...?

Well, operator-- will fail only in some specific cases, e.g.:

• try_decrement fails due to concurrent erase (it can be the element pointed to by the iterator)
• operator-- calls lower_boud which returns end() (the tree is now empty)
• try_decrement crashes because we decerement beyond begin()

So, one idea for a test would be to just have a single element in the tree and in one thread call find() and then operator--, in another thread, just remove this element. Probably this should be done in a loop.

[lukaszstolarczuk]

Reviewable status: 0 of 9 files reviewed, 1 unresolved discussion (waiting on @igchor)

---

tests/radix_tree/radix_concurrent_erase.cpp, line 104 at r3 (raw file):

Previously, igchor (Igor Chorążewicz) wrote…

Well, operator-- will fail only in some specific cases, e.g.:

• try_decrement fails due to concurrent erase (it can be the element pointed to by the iterator)
• operator-- calls lower_boud which returns end() (the tree is now empty)
• try_decrement crashes because we decerement beyond begin()

So, one idea for a test would be to just have a single element in the tree and in one thread call find() and then operator--, in another thread, just remove this element. Probably this should be done in a loop.

I've done it a bit differently with random removals - now it fails randomly

[igchor]

Reviewed 3 of 9 files at r5.
Reviewable status: 3 of 9 files reviewed, all discussions resolved (waiting on @igchor)

[lukaszstolarczuk]

as discussed I switched implementation, to disable operator-- when MtMode is enabled

Reviewable status: 3 of 9 files reviewed, all discussions resolved (waiting on @igchor)

[igchor]

Reviewable status: 3 of 9 files reviewed, all discussions resolved (waiting on @igchor)

[lukaszstolarczuk]

Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @igchor)

---

tests/radix_tree/radix_concurrent_iterate.cpp, line 249 at r8 (raw file):

		middle_key.end());

	auto last = std::next(ptr->begin(), static_cast<int>(ptr->size() - 1));

I'm not sure if this is exactly what we wanted here? Will the find & erase (below) do the job as we wanted?

[igchor]

Reviewable status: 3 of 9 files reviewed, 1 unresolved discussion (waiting on @igchor and @lukaszstolarczuk)

---

tests/radix_tree/radix_concurrent_iterate.cpp, line 249 at r8 (raw file):

Previously, lukaszstolarczuk (Łukasz Stolarczuk) wrote…

I'm not sure if this is exactly what we wanted here? Will the find & erase (below) do the job as we wanted?

Yes, I think it's the same.