radix_tree: use std::atomic<tagged_pointer>

[JanDorniak99]

This change is 

[codecov]

Codecov Report

Merging #1069 (e8de081) into master (485353b) will decrease coverage by 0.17%.
The diff coverage is 99.55%.

@@            Coverage Diff             @@
##           master    #1069      +/-   ##
==========================================
- Coverage   94.08%   93.90%   -0.18%     
==========================================
  Files          48       49       +1     
  Lines        4751     4726      -25     
==========================================
- Hits         4470     4438      -32     
- Misses        281      288       +7     

Flag	Coverage Δ
tests_clang_debug_cpp17	93.29% <99.54%> (-0.18%)	⬇️
tests_gcc_debug	91.45% <90.00%> (-0.49%)	⬇️

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

Impacted Files	Coverage Δ
include/libpmemobj++/detail/common.hpp	68.18% <ø> (-6.82%)	⬇️
...memobj++/experimental/atomic_self_relative_ptr.hpp	100.00% <ø> (ø)
include/libpmemobj++/experimental/radix_tree.hpp	99.40% <99.31%> (-0.42%)	⬇️
include/libpmemobj++/detail/tagged_ptr.hpp	100.00% <100.00%> (ø)
...de/libpmemobj++/experimental/self_relative_ptr.hpp	100.00% <100.00%> (ø)
include/libpmemobj++/container/vector.hpp	91.25% <0.00%> (-1.05%)	⬇️
include/libpmemobj++/container/segment_vector.hpp	93.89% <0.00%> (-0.22%)	⬇️
...ude/libpmemobj++/container/concurrent_hash_map.hpp	93.88% <0.00%> (-0.16%)	⬇️
... and 4 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 485353b...e8de081. Read the comment docs.

[igchor]

Reviewed 1 of 3 files at r1, 3 of 3 files at r2.
Reviewable status: all files reviewed, 4 unresolved discussions (waiting on @JanDorniak99)

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 13 at r2 (raw file):

#include <libpmemobj++/persistent_ptr.hpp>

// XXX use this from atomic_self_relative_ptr.hpp

Could you please move this code to some common file and use from here and self_relative_ptr?

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 256 at r2 (raw file):

	store_with_snapshot(
		value_type desired,
		std::memory_order order = std::memory_order_seq_cst) noexcept

it's not noexcept unfortunately - transaction::snapshot can throw

---

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

	struct node;

	using pointer_type = std::atomic<detail::tagged_ptr<leaf, node>>;

I think the other way around would be more readable:
atomic_pointer_type and pointer_type

---

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

	while (!is_leaf(n)) {
		if (n->embedded_entry.load() && n->byte >= min_depth)

All the loads should be called with std::memory_order_acquire and stores with std::memory_order_release. Maybe it would make sense to implement some wrapper for load/store to avoid so much boilerplate? Something like load_acquire(...), store_release()

More info about those orders: https://en.cppreference.com/w/cpp/atomic/memory_order

Btw, in some places std::memory_order_relaxed would be enough but I'm not sure if it's worth complication the code and using different memor orders

[KFilipek]

Reviewed 3 of 3 files at r2.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @JanDorniak99)

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 49 at r2 (raw file):

Quoted 6 lines of code…

namespace std
{
template <typename P1, typename P2>
struct atomic<pmem::detail::tagged_ptr_impl<
	P1, P2, pmem::obj::experimental::self_relative_ptr<void>>>;
}

Why not move it to below namespace pmem::detail, now we have splitted file four times: pmem::detail and again std

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 218 at r2 (raw file):

namespace std
{

@igchor should we expose it in std namespace or in pmem?

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 223 at r2 (raw file):

struct atomic<pmem::detail::tagged_ptr_impl<
	P1, P2, pmem::obj::experimental::self_relative_ptr<void>>> {

Is short version work either?:
struct atomic<detail::pmem::tagged_ptr>
Why not use what we defined earlier.

---

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

Quoted 7 lines of code…

		for (size_t i = 0; i < SLNODES; i++) {
			non_atomic_pointer_type m;
			if ((m = n->child[i].load())) {
				n = m;
				break;
			}
		}

n, m, i made this code unreadable

[igchor]

Reviewable status: all files reviewed, 7 unresolved discussions (waiting on @JanDorniak99)

---

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

Previously, KFilipek (Krzysztof Filipek) wrote…

		for (size_t i = 0; i < SLNODES; i++) {
			non_atomic_pointer_type m;
			if ((m = n->child[i].load())) {
				n = m;
				break;
			}
		}

n, m, i made this code unreadable

Probably we could just change this to:

if (n->child[i].load())
    return get_leaf(n->child[i].load())

As for n and i imo it's fine n is used throughout the entire implementation as pointer to node and i is just a standard choice for loops.

[JanDorniak99]

Reviewable status: 2 of 8 files reviewed, 7 unresolved discussions (waiting on @igchor, @JanDorniak99, and @KFilipek)

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 13 at r2 (raw file):

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

Could you please move this code to some common file and use from here and self_relative_ptr?

Done.

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 49 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

namespace std
{
template <typename P1, typename P2>
struct atomic<pmem::detail::tagged_ptr_impl<
	P1, P2, pmem::obj::experimental::self_relative_ptr<void>>>;
}

Why not move it to below namespace pmem::detail, now we have splitted file four times: pmem::detail and again std

Done.

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 223 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

struct atomic<pmem::detail::tagged_ptr_impl<
	P1, P2, pmem::obj::experimental::self_relative_ptr<void>>> {

Is short version work either?:
struct atomic<detail::pmem::tagged_ptr>
Why not use what we defined earlier.

Done.

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 256 at r2 (raw file):

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

it's not noexcept unfortunately - transaction::snapshot can throw

Done.

---

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

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

I think the other way around would be more readable:
atomic_pointer_type and pointer_type

Done.

---

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

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

All the loads should be called with std::memory_order_acquire and stores with std::memory_order_release. Maybe it would make sense to implement some wrapper for load/store to avoid so much boilerplate? Something like load_acquire(...), store_release()

More info about those orders: https://en.cppreference.com/w/cpp/atomic/memory_order

Btw, in some places std::memory_order_relaxed would be enough but I'm not sure if it's worth complication the code and using different memor orders

Done.

[KFilipek]

Reviewed 1 of 3 files at r1, 6 of 6 files at r3.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on @igchor and @JanDorniak99)

[igchor]

Reviewed 2 of 6 files at r3.
Reviewable status: all files reviewed, 1 unresolved discussion (waiting on @JanDorniak99)

---

include/libpmemobj++/detail/tagged_ptr.hpp, line 218 at r2 (raw file):

Previously, KFilipek (Krzysztof Filipek) wrote…

namespace std
{

@igchor should we expose it in std namespace or in pmem?

In std, this is a common way of providing atomic stores/loads for custom data types. Btw, @JanDorniak99 please look at this and see if we fulfill all requirements for specialization: https://en.cppreference.com/w/cpp/language/extending_std I mean those: "Specializations of std::atomic must have a deleted copy constructor, a deleted copy assignment operator, and a constexpr value constructor."

---

tests/radix_tree/radix_concurrent.cpp, line 18 at r3 (raw file):

		    size_t n_readers)
{
	std::vector<std::thread> threads;

nit: you could probably just use parallel_exec here (if (id == 0) WriteF(); else readers[id]();) or even use parallel_exec directly in tests

[karczex]

---

include/libpmemobj++/experimental/self_relative_ptr.hpp, line 31 at r4 (raw file):

	using base_type = self_relative_ptr_base;
	using this_type = self_relative_ptr;

What's the point of those two usings? Is it some part of API, which documentation I cannot find?

[igchor]

Reviewable status: 5 of 8 files reviewed, 1 unresolved discussion (waiting on @igchor, @karczex, and @KFilipek)

---

include/libpmemobj++/experimental/self_relative_ptr.hpp, line 31 at r4 (raw file):

Previously, karczex (Paweł Karczewski) wrote…

	using base_type = self_relative_ptr_base;
	using this_type = self_relative_ptr;

What's the point of those two usings? Is it some part of API, which documentation I cannot find?

Right, I guess those code be made private

[JanDorniak99]

Reviewable status: 5 of 8 files reviewed, 1 unresolved discussion (waiting on @igchor, @karczex, and @KFilipek)

---

include/libpmemobj++/experimental/self_relative_ptr.hpp, line 31 at r4 (raw file):

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

Right, I guess those code be made private

Done.

[igchor]

Reviewable status: 5 of 8 files reviewed, 1 unresolved discussion (waiting on @igchor, @karczex, and @KFilipek)

---

include/libpmemobj++/experimental/self_relative_ptr.hpp, line 31 at r4 (raw file):

Previously, JanDorniak99 (Jan Dorniak) wrote…

Done.

I don't see any change, did you push changes?

[igchor]

Reviewed 1 of 1 files at r5.
Reviewable status: 6 of 8 files reviewed, 1 unresolved discussion (waiting on @karczex and @KFilipek)