Persistent concurrent hash map data structure.

[vinser52]

Implementation is based on concurrent_hash_map from Intel TBB library.

Test for persistent_concurrent_hash_map is built and executed only if
Intel TBB library found by CMake.

---

This change is 

[pbalcer]

Run it through clang format, and also please cleanup defines/names to not include 'TBB' apart from included stuff.

[pbalcer]

please fix the copyright header.

[pbalcer]

It's not a TBB specific thing ;)

[pbalcer]

pmem

[pbalcer]

just make a persistent_pool_ptr constructor in persistent_ptr.

[pbalcer]

we should think about making some kind of base template, which would take the type of the pointer as a template parameter.
As is, there is a lot of copy/pasted boilerplate code.

[pbalcer]

#if DEBUG
do we need this though? I think we should just remove these traces.

[pbalcer]

can't you just make tmp_node a persistent pool ptr?

[pbalcer]

we should probably unify the interfaces with this functionality. in the array it's slice

[pbalcer]

just remove everything that's unnecessary.

[pbalcer]

why 512 and 2? make it static const variable.

[marcinslusarz]

1. Please make sure all Travis/AppVeyor checks pass.
2. New code should be tested on Travis. This means TBB needs to be installed on our Docker images (see utils/docker/). I'm not sure what to do about AppVeyor...

[igchor]

Reviewed 5 of 7 files at r1.
Reviewable status: 5 of 7 files reviewed, 22 unresolved discussions (waiting on @pbalcer, @igchor, and @vinser52)

---

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

*/

#ifndef __PMEM_persistent_concurrent_hash_map_H

I think it would be good too use the same convention as in the rest of header files:
PMEMOBJ_PERSISTENT_CONCURRENT_HASH_MAP_HPP

---

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

    inline T itt_hide_load_word( const tbb::atomic<T>& src ) {
#if TBB_USE_THREADING_TOOLS
        //TODO: This assertion should be replaced with static_assert

so why is it not static_assert?

---

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

        typedef persistent_hash_map_node_base node_base;
        //! Node base pointer
        typedef persistent_pool_ptr<node_base> node_base_ptr_t;

Why only node_base_ptr_t is persistent_pool_ptr? What about tmp_node_ptr_t, segment_ptr_t, bucket_ptr_t?

---

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

Previously, pbalcer (Piotr Balcer) wrote…

PMEMOBJ_MAX_ALLOC_SIZE;

constexpr?

---

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

    friend bool is_write_access_needed( accessor_not_used const& ) { return false; }

#if __TBB_CPP11_RVALUE_REF_PRESENT

I think this check can be removed, for all bindings we require C++11 support.

---

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

    hashcode_t m = (hashcode_t)itt_load_word_with_acquire( internal::as_atomic( my_mask.get_ro() ) );
    pool_base p = get_pool_base();
    // NVML could throw transaction error exaption, do we need to catch them or it is user resposibility

exception.

I would say that if you start the transaction then it's your responsibility.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 31 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

pmem

pmem::obj::experimental

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 452 at r1 (raw file):

operator!=( const persistent_pool_ptr<T> &lhs, std::nullptr_t ) noexcept
{
    return lhs.raw() != 0;

I think it would be better to use nullptr instead of 0.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 462 at r1 (raw file):

operator==( const persistent_pool_ptr<T> &lhs, std::nullptr_t ) noexcept
{
    return lhs.raw() == 0;

nullptr

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 472 at r1 (raw file):

operator==( std::nullptr_t, const persistent_pool_ptr<T> &lhs ) noexcept
{
    return lhs.raw() == 0;

nullptr

---

tests/CMakeLists.txt, line 233 at r1 (raw file):

if(TBB_FOUND)
	build_test(persistent_concurrent_hash_map persistent_concurrent_hash_map/persistent_concurrent_hash_map.cpp)
	add_test_generic(persistent_concurrent_hash_map none)

Probably we should also test it under pmemcheck.

[vinser52]

Reviewable status: 5 of 7 files reviewed, 22 unresolved discussions (waiting on @pbalcer, @vinser52, and @igchor)

---

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

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

I think it would be good too use the same convention as in the rest of header files:
PMEMOBJ_PERSISTENT_CONCURRENT_HASH_MAP_HPP

Done.

---

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

Previously, pbalcer (Piotr Balcer) wrote…

we should probably name this pmem::obj::experimental::concurrent_unordered_map

I have changed namespace to pmem::obj::experimental from pmem::experimental, but oncurrent_unordered_map is inappropriate because in TBB we have concurrent_hash_map and concurrent_unordered_map. It is two different implementations of unordered maps :).

---

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

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

so why is it not static_assert?

Done.

---

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

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

Why only node_base_ptr_t is persistent_pool_ptr? What about tmp_node_ptr_t, segment_ptr_t, bucket_ptr_t?

Because I use tmp_node_ptr_t, segment_ptr_t, bucket_ptr_t to allocate persistent memory using make_persistent_atomic(). This function does not accept persistent_pool_ptr

---

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

DEBUG
Actually TRACEP might be useful to analyze multi-threading issues. But if you what I can remove it.

---

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

Previously, pbalcer (Piotr Balcer) wrote…

can't you just make tmp_node a persistent pool ptr?

No. Because I pass it to make_persistent_atomic(...) which requires persistent_ptr

---

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

Previously, pbalcer (Piotr Balcer) wrote…

we should probably unify the interfaces with this functionality. in the array it's slice

Actually hash_map_range used by TBB parallel algorithms to split range to the chunks that could be processed as a separate tasks

---

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

Previously, pbalcer (Piotr Balcer) wrote…

just remove everything that's unnecessary.

Done.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 2 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

please fix the copyright header.

Done.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 21 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

It's not a TBB specific thing ;)

Done.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 31 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

pmem

Done.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 38 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

persistent_slim_ptr ?

I agree. But I think it subject of separate pull request because it will impacts persistent_ptr.

[vinser52]

Reviewable status: 5 of 7 files reviewed, 22 unresolved discussions (waiting on @pbalcer, @vinser52, and @igchor)

---

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

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

constexpr?

Done.

---

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

Previously, pbalcer (Piotr Balcer) wrote…

allocate/deallocate? What are we enabling or disabling?

We are enabling and disabling corresponding segment.

---

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

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

I think this check can be removed, for all bindings we require C++11 support.

Done.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 452 at r1 (raw file):

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

I think it would be better to use nullptr instead of 0.

No. In this case raw() returns const uint64_t and there is no operator!=(...) which takes const uint64_t and std::nullptr_t.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 462 at r1 (raw file):

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

nullptr

No. In this case raw() returns const uint64_t and there is no operator!=(...) which takes const uint64_t and std::nullptr_t.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 472 at r1 (raw file):

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

nullptr

No. In this case raw() returns const uint64_t and there is no operator!=(...) which takes const uint64_t and std::nullptr_t.

[pmem-bot]

@lplewa and @kkajrewicz please review this pull request

[igchor]

Reviewed 1 of 7 files at r1.
Reviewable status: 6 of 7 files reviewed, 15 unresolved discussions (waiting on @pbalcer and @vinser52)

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

Because I use tmp_node_ptr_t, segment_ptr_t, bucket_ptr_t to allocate persistent memory using make_persistent_atomic(). This function does not accept persistent_pool_ptr

Ok, but as I understand using persistent_pool_ptr is a memory optimization.
Wouldn't it make sense to convert persistent_ptr from make_persistent() to persistnet_pool_ptr (and back if neccessary)?

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

Done.

Done?

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 31 at r1 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Done.

Done?

[igchor]

Could you rebase to current master? We have updated images to ubuntu 18.04 and fedora 28 some time ago.

[marcinslusarz]

Please rebase your PR (don't merge master back!) and squash changes into fewer commits.

[igchor]

Could you please you add this 2 line to main CMakeLists.txt:

add_cppstyle(include-experimental ${CMAKE_CURRENT_SOURCE_DIR}/include/libpmemobj++/experimental/*.hpp)
add_check_whitespace(include-experimental ${CMAKE_CURRENT_SOURCE_DIR}/include/libpmemobj++/experimental/*.hpp)

And reformat the code then?

[vinser52]

Reviewable status: 1 of 12 files reviewed, 15 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

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

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

Ok, but as I understand using persistent_pool_ptr is a memory optimization.
Wouldn't it make sense to convert persistent_ptr from make_persistent() to persistnet_pool_ptr (and back if neccessary)?

I need persistent_ptr to call make_persistent_atomic() function. I need to pass this pointer to the function as a parameter. maybe I did not get your idea. Could you please clarify your idea?

---

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

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

Done?

Done

---

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

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

exception.

I would say that if you start the transaction then it's your responsibility.

I have redesigned this part of code.

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 286 at r1 (raw file):

Previously, pbalcer (Piotr Balcer) wrote…

just make a persistent_pool_ptr constructor in persistent_ptr.

I suggest to leave it as is until we re-design this code and create persistent_slim_ptr.

[vinser52]

Reviewable status: 1 of 12 files reviewed, 15 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

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

Previously, pbalcer (Piotr Balcer) wrote…

why 512 and 2? make it static const variable.

Just removed this diagnostic code

---

include/libpmemobj++/experimental/persistent_pool_ptr.h, line 31 at r1 (raw file):

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

Done?

Done

[igchor]

Reviewed 3 of 8 files at r2, 7 of 11 files at r3.
Reviewable status: 10 of 12 files reviewed, 15 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

I need persistent_ptr to call make_persistent_atomic() function. I need to pass this pointer to the function as a parameter. maybe I did not get your idea. Could you please clarify your idea?

Ok, I see that, you are right, this must be a persistent_ptr.

[igchor]

Reviewable status: 9 of 12 files reviewed, 16 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

tests/ctest_helpers.cmake, line 139 at r3 (raw file):

	add_executable(${name} ${srcs})
	target_link_libraries(${name} ${PMEMOBJ_LIBRARIES} ${CMAKE_THREAD_LIBS_INIT} ${TBB_IMPORTED_TARGETS})

Linking TBB to all tests is probably not a good idea.
I would suggest creating wrapper function for tbb like this:

function(build_test_tbb name)
    build_test(${name} ${ARGN})
    target_link_libraries(${name} ${TBB_IMPORTED_TARGETS})
endfunction()

This would probably fix failing tests on travis.

[vinser52]

Reviewable status: 9 of 12 files reviewed, 16 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

tests/ctest_helpers.cmake, line 139 at r3 (raw file):

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

Linking TBB to all tests is probably not a good idea.
I would suggest creating wrapper function for tbb like this:

function(build_test_tbb name)
    build_test(${name} ${ARGN})
    target_link_libraries(${name} ${TBB_IMPORTED_TARGETS})
endfunction()

This would probably fix failing tests on travis.

I did this changes, and it will fix non-TBB tests. But, I believe, hash map test will fail since it should be linked with TBB. Is the way to diagnose the reason why test failed? I guess that system loader might not find libtbb.so. But on my local machine test works fine.

[vinser52]

Reviewable status: 9 of 12 files reviewed, 16 unresolved discussions (waiting on @igchor and @pbalcer)

---

tests/CMakeLists.txt, line 233 at r1 (raw file):

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

Probably we should also test it under pmemcheck.

Done

[igchor]

Reviewable status: 7 of 12 files reviewed, 15 unresolved discussions (waiting on @igchor and @pbalcer)

---

tests/ctest_helpers.cmake, line 139 at r3 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

I did this changes, and it will fix non-TBB tests. But, I believe, hash map test will fail since it should be linked with TBB. Is the way to diagnose the reason why test failed? I guess that system loader might not find libtbb.so. But on my local machine test works fine.

Okay, this might have something to do with using tbb and custom libcxx in one of builds (in run-build.sh). Please try enabling tbb only for cases where -DUSE_LLVM_LIBCPP=0.

[vinser52]

Reviewable status: 7 of 12 files reviewed, 15 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

tests/ctest_helpers.cmake, line 139 at r3 (raw file):

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

Okay, this might have something to do with using tbb and custom libcxx in one of builds (in run-build.sh). Please try enabling tbb only for cases where -DUSE_LLVM_LIBCPP=0.

How did you recognize that there is a conflict between libcxx and tbb?

[igchor]

Reviewable status: 7 of 12 files reviewed, 15 unresolved discussions (waiting on @igchor, @pbalcer, and @vinser52)

---

utils/docker/run-build.sh, line 91 at r4 (raw file):

			-DUSE_LLVM_LIBCPP=1 \
			-DLIBCPP_LIBDIR=$LIBCPP_LIBDIR \
			-DLIBCPP_INCDIR=$LIBCPP_INCDIR \

Please remove .
Also not specifiing TBB_DIR does not mean CMake won't find TBB. I suggest adding some additional CMake variable like USE_TBB.

[kkajrewicz]

Reviewed 1 of 7 files at r1, 2 of 8 files at r2, 5 of 11 files at r3, 4 of 4 files at r4.
Reviewable status: all files reviewed, 44 unresolved discussions (waiting on @pbalcer, @igchor, and @vinser52)

---

README.md, line 54 at r4 (raw file):

```sh
$ ...
$ cmake .. -DTBB_DIR=<Path to Intel TBB>/cmake

On tbb master I ran:
cmake .. -DTBB_DIR=/home/kkajre/Repos/tbb/cmake/ ,but CMake fails to detect TBB library:
(...)
CMake Warning at tests/CMakeLists.txt:274 (message):
Skipping persistent_concurrent_hash_map test because Intel TBB library not
found
(...)

It seems to work when I run aforementioned command on released package e.g. https://github.com/01org/tbb/releases/download/2018_U5/tbb2018_20180618oss_lin.tgz

---

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

		tmp_node_ptr_t tmp_node;
	};
	//! Count of segments in the first block

update this and the next comment

---

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

	p<uint64_t> my_pool_uuid;
	//! Hash mask = sum of allocated segment sizes - 1
	p<hashcode_t> my_mask;

1. use in class initialization
2. add const

---

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

	segments_table_t my_table;
	//! Size of container in stored items
	p<size_type> my_size; // It must be in separate cache line from my_mask

use in class initialization

---

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

	segment_size(segment_index_t k)
	{
		return size_type(1) << k; // fake value for k==0

k == 0 / i.e. add spacing

---

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

	class segment_traits_t {
	public:
		//! PMDK has limitation for allocation size. It is 16 Gb now.

s/Gb/GB. Btw. I wonder if you should write its value explicitly in the comment. If this constant will change one day I'm pretty sure you will forget to update it here that might be misleading for someone reading your code.

---

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

						     : first_big_block +
					(size_t(1) << (seg - first_big_block)) -
					1;

move it to the line above

---

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

		typedef typename std::conditional<is_const, const BlockTable &,
						  BlockTable &>::type
			table_reference;

ditto

---

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

		typedef typename std::conditional<is_const, const BlockTable *,
						  BlockTable *>::type
			table_pointer;

ditto

---

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

		bucket &operator[](size_t i) const
		{
			__TBB_ASSERT(i < size(), "Index exceed segment size");

exceeds

---

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

		void
		enable(pool_base &pop)

const pool_base &pop

---

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

		typedef std::pair<size_t, size_t> block_range;

		// returns blocks [begin, end) for corrsponding segment

typo - corresponding

---

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

			     "on persistent memory");
		my_pool_uuid = oid.pool_uuid_lo;
		for (size_type i = 0; i < embedded_block; i++) // fill the table

++i

---

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

		__TBB_ASSERT(is_valid(b->tmp_node), NULL);
		__TBB_ASSERT(b->tmp_node->next == b->node_list, NULL);
		b->node_list = b->tmp_node; // its under lock and flag is set

it's locked

---

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

			sz <<= 1; // double it to get entire capacity of the
				  // container
		} else {	  // the first block

is this comment valid? under the first condition you check 'k >= first block' so the first block will be handled there I guess

---

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

				  // container
		} else {	  // the first block
			// TODO: refactor this code to incapsulate logic in

encapsulate

---

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

	// Splitting into two functions should help inlining
	inline bool
	check_mask_race(const hashcode_t h, hashcode_t &m) const

qualifying hashcode_t h with const seems redundant as you copy it and pass directly to another func.

---

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

	{
		if (b->tmp_node != nullptr) {
			if (b->tmp_node->next == b->node_list) // Than

useless comment

---

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

					     .is_valid(),
				     "new allocations must not publish new "
				     "mask until segment has allocated");

has been

---

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

	reserve(size_type buckets)
	{
		if (!buckets--)

condition will be satisfied if buckets == 1, do you mean that? if so, why not to simply checking that exact condition and decrement it later when condtion is not fullfilled?

---

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

		this->my_mask.swap(table.my_mask);
		this->my_size.swap(table.my_size);
		for (size_type i = 0; i < embedded_buckets; i++)

++i

---

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

			this->my_embedded_segment[i].node_list.swap(
				table.my_embedded_segment[i].node_list);
		for (size_type i = embedded_block; i < pointers_per_table; i++)

ditto

---

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

	{
		return const_cast<persistent_concurrent_hash_map *>(this)
			->lookup(/*insert*/ false, key, NULL, NULL,

s/NULL/nullptr in this and subsequent lookup() calls

---

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

		return const_cast<persistent_concurrent_hash_map *>(this)
			->lookup(/*insert*/ false, key, NULL, &result,
				 /*write=*/false, &do_not_allocate_node);

remove '=' from write comment to be consistent with 'insert' above; add spacing after comment; Fix subsequent methods as well

---

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

	accessor_location(accessor_not_used const &)
	{
		return NULL;

nullptr

---

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

		b); // only the last segment should be scanned for rehashing

	for (; b <= mask; b++, bp++) {

++b, ++bp

---

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

	pool_base p = get_pool_base();
	{ // transaction scope
		// PMDK could throw transaction error exaption, do we need to

typo - exception

---

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

	{ // transaction scope
		// PMDK could throw transaction error exaption, do we need to
		// catch them or it is user resposibility

typo - responsibility

---

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

				     "wrong mask or concurrent grow");
			size_type sz = segment.size();
			for (segment_index_t i = 0; i < sz; i++)

++i

[igchor]

Reviewable status: all files reviewed, 45 unresolved discussions (waiting on @pbalcer, @igchor, and @vinser52)

---

utils/docker/run-build.sh, line 91 at r4 (raw file):

			-DUSE_LLVM_LIBCPP=1 \
			-DLIBCPP_LIBDIR=$LIBCPP_LIBDIR \
			-DLIBCPP_INCDIR=$LIBCPP_INCDIR \

I meant, remove backslash at the end of file.

[vinser52]

Reviewable status: all files reviewed, 45 unresolved discussions (waiting on @pbalcer, @igchor, @vinser52, and @kkajrewicz)

---

README.md, line 54 at r4 (raw file):

Previously, kkajrewicz wrote…

On tbb master I ran:
cmake .. -DTBB_DIR=/home/kkajre/Repos/tbb/cmake/ ,but CMake fails to detect TBB library:
(...)
CMake Warning at tests/CMakeLists.txt:274 (message):
Skipping persistent_concurrent_hash_map test because Intel TBB library not
found
(...)

It seems to work when I run aforementioned command on released package e.g. https://github.com/01org/tbb/releases/download/2018_U5/tbb2018_20180618oss_lin.tgz

Yes. This is only work with pre-built release package of TBB. We are not going to build TBB from sources.

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

Just removed this diagnostic code

Done.

---

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

Previously, kkajrewicz wrote…

update this and the next comment

Done.

---

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

Previously, kkajrewicz wrote…

1. use in class initialization
2. add const

my_mask is non-const member.

---

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

Previously, kkajrewicz wrote…

use in class initialization

Done.

---

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

Previously, kkajrewicz wrote…

k == 0 / i.e. add spacing

Done.

---

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

Previously, kkajrewicz wrote…

s/Gb/GB. Btw. I wonder if you should write its value explicitly in the comment. If this constant will change one day I'm pretty sure you will forget to update it here that might be misleading for someone reading your code.

Done. I changed comments to not mention exact limit size

---

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

Previously, kkajrewicz wrote…

move it to the line above

clang-format formatted code in that way. If I change it than cmake build will fail.

---

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

Previously, kkajrewicz wrote…

ditto

clang-format formatted code in that way. If I change it than cmake build will fail.

---

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

Previously, kkajrewicz wrote…

ditto

clang-format formatted code in that way. If I change it than cmake build will fail.

---

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

Previously, kkajrewicz wrote…

exceeds

Done.

---

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

Previously, kkajrewicz wrote…

const pool_base &pop

No. make_persistent_atomic requires non-const reference.

---

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

Previously, kkajrewicz wrote…

typo - corresponding

Done.

---

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

Previously, kkajrewicz wrote…

++i

Done.

---

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

Previously, kkajrewicz wrote…

it's locked

Done.

---

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

Previously, kkajrewicz wrote…

is this comment valid? under the first condition you check 'k >= first block' so the first block will be handled there I guess

first_block is an upper bound of the segment indexes that fits to first block. In our case first_block = 8, it means that segments [0..8) is a first block, and starting from index 8 it is not a first block.

---

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

Previously, kkajrewicz wrote…

encapsulate

Done.

---

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

Previously, kkajrewicz wrote…

qualifying hashcode_t h with const seems redundant as you copy it and pass directly to another func.

Done.

---

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

Previously, kkajrewicz wrote…

useless comment

Done. Removed.

---

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

Previously, kkajrewicz wrote…

has been

Done.

---

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

Previously, kkajrewicz wrote…

condition will be satisfied if buckets == 1, do you mean that? if so, why not to simply checking that exact condition and decrement it later when condtion is not fullfilled?

No. It is postfix decrement. This condition will true if bucket == 0.

---

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

Previously, kkajrewicz wrote…

++i

Done.

---

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

Previously, kkajrewicz wrote…

ditto

Done.

---

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

Previously, kkajrewicz wrote…

s/NULL/nullptr in this and subsequent lookup() calls

Done.

---

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

Previously, kkajrewicz wrote…

remove '=' from write comment to be consistent with 'insert' above; add spacing after comment; Fix subsequent methods as well

Done.

---

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

Previously, kkajrewicz wrote…

nullptr

Done.

---

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

Previously, kkajrewicz wrote…

++b, ++bp

Done.

---

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

Previously, kkajrewicz wrote…

typo - exception

Done.

---

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

Previously, kkajrewicz wrote…

typo - responsibility

Done.

---

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

Previously, kkajrewicz wrote…

++i

Done.

---

utils/docker/run-build.sh, line 91 at r4 (raw file):

Previously, igchor wrote…

I meant, remove backslash at the end of file.

Done.

[vinser52]

Reviewable status: all files reviewed, 45 unresolved discussions (waiting on @pbalcer, @igchor, and @kkajrewicz)

---

utils/docker/run-build.sh, line 91 at r4 (raw file):

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

Please remove .
Also not specifiing TBB_DIR does not mean CMake won't find TBB. I suggest adding some additional CMake variable like USE_TBB.

Done.

[igchor]

Reviewed 1 of 11 files at r3, 1 of 5 files at r5.
Reviewable status: 8 of 12 files reviewed, 44 unresolved discussions (waiting on @igchor, @kkajrewicz, @pbalcer, and @vinser52)

---

include/libpmemobj++/experimental/persistent_pool_ptr.hpp, line 522 at r5 (raw file):

template <class T, class U>
persistent_pool_ptr<T>
static_persistent_pool_pointer_cast(const persistent_pool_ptr<U> &r)

Why is this needed? Why not just rely on copy construct and assignment operators for convertible types?

[igchor]

Reviewed 3 of 5 files at r5.
Reviewable status: 11 of 12 files reviewed, 44 unresolved discussions (waiting on @kkajrewicz, @pbalcer, @igchor, and @vinser52)

[vinser52]

Reviewable status: 11 of 12 files reviewed, 44 unresolved discussions (waiting on @kkajrewicz, @pbalcer, @igchor, and @vinser52)

---

include/libpmemobj++/experimental/persistent_pool_ptr.hpp, line 522 at r5 (raw file):

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

Why is this needed? Why not just rely on copy construct and assignment operators for convertible types?

Because I need static_cast functionality for persistent pointers. In my case I have node class which derives from node_base class. And I need to cast node_base* to node*. With regular pointers we use static cast in the following way:
node* n = static_cast<node_base*>(base_ptr);

So, I introduce the same for persistent_pool_ptr. For example, std::shared_ptr has the same functionality.

[igchor]

Reviewable status: 11 of 12 files reviewed, 42 unresolved discussions (waiting on @kkajrewicz and @pbalcer)

---

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

Previously, vinser52 (Sergei Vinogradov) wrote…

I have redesigned this part of code.

Ok, but still I think when you start a transaction, user should not get a transacion_error or any other transaction conncted exception.
I think whenever you use transaction you should catch every exception and rethrow some custom one (and maybe do some other things to have consistent state).

Maybe logic like this would be better?:

if (pmemobj_tx_stage() == TX_STAGE_WORK)
    // rethrow original exception
else
   throw custom_exception

This would propagate erorr if there is outer transaction and throw custom one otherwise. This comment also applies to other places where transactions are used.

---

include/libpmemobj++/experimental/persistent_pool_ptr.hpp, line 522 at r5 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Because I need static_cast functionality for persistent pointers. In my case I have node class which derives from node_base class. And I need to cast node_base* to node*. With regular pointers we use static cast in the following way:
node* n = static_cast<node_base*>(base_ptr);

So, I introduce the same for persistent_pool_ptr. For example, std::shared_ptr has the same functionality.

Ok, but is really necessary in this case?
Couldn't you just always use persiststent_ptr instead of persiststent_ptr<node_base>?

[vinser52]

Reviewable status: 4 of 21 files reviewed, 30 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 230 at r21 (raw file):

Previously, lplewa (Łukasz Plewa) wrote…

#else
#error "missing pause instruction (...)"
#endif

I am not sure that we should emit error. Without pause the code would be just less efficient.

[vinser52]

Reviewable status: 4 of 21 files reviewed, 30 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 178 at r21 (raw file):

Previously, lplewa (Łukasz Plewa) wrote…

this variant of function returns unsigned long, where others returns int

Log2 just a thin wrapper around built-in functions. This is done because Microsoft and GCC built-ins return different type.

[igchor]

Reviewed 1 of 6 files at r24.
Reviewable status: 5 of 21 files reviewed, 22 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 56 at r24 (raw file):

```sh
$ ...
$ cmake .. -DTBB_DIR=<Path to Intel TBB>/cmake

I think this snippet is redundant - this is already wrote few lines above.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 178 at r21 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Log2 just a thin wrapper around built-in functions. This is done because Microsoft and GCC built-ins return different type.

I think in this case you should cast the return value to some one type - probably cast MSC version to int and maybe add some assert to make sure there is no overflow.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 1646 at r24 (raw file):

} // namespace internal
//! @endcond

Could you change the comments starting with "//" to "/**/" ? So that it would be consistent with other files.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 1827 at r24 (raw file):

				}
			} else {
				bucket::scoped_t::acquire(my_b->mutex, writer);

what we jumped to the code in "else" and just before scoped_t::acquire() some other thread inserted some element and enabled new segment so that bucket now require rehahsing? Is this scenario even possible?

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2781 at r24 (raw file):

		goto check_growth;

	// TODO: the following seems as generic/regular operation

So what is TODO here?

---

include/libpmemobj++/experimental/persistent_pool_ptr.hpp, line 33 at r24 (raw file):

 */

#ifndef PMEMOBJ_PERSISTENT_POOL_PTR_HPP

Please move persistent_poo_ptr.hpp to detail folder and namespace, in future we could replace it with self-relative pointer and we don;'t really need to expose this pointer to the user.

[igchor]

Reviewable status: 5 of 21 files reviewed, 23 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 37 at r24 (raw file):

#### To build packages ####

Please remove this newline.

[vinser52]

Reviewable status: 5 of 21 files reviewed, 23 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 37 at r24 (raw file):

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

Please remove this newline.

I do not see the line number for this comments

---

README.md, line 56 at r24 (raw file):

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

I think this snippet is redundant - this is already wrote few lines above.

I do not see the line number for this comments

[vinser52]

Reviewable status: 5 of 21 files reviewed, 23 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 37 at r24 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

I do not see the line number for this comments

Done. I think I found it

---

README.md, line 56 at r24 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

I do not see the line number for this comments

Did you mean remove example "cmake ... -DTBB_DIR ..."?

[vinser52]

Reviewable status: 5 of 21 files reviewed, 23 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 1352 at r20 (raw file):

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

in lookup method I see you don';t even check the return value. So what happens if this evalutaes to false - it means that another thread enabled it?

You are right. We pass std::try_to_lock to unique_lock. Only one thread will succeed. In case of 'false' it means that another thread in progress with enabling new segment. And once the new segment is enabled the mask is increased - it allows to guarantee that no one will access new segment until it is fully enabled.

[igchor]

Reviewable status: 5 of 21 files reviewed, 21 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 56 at r24 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Did you mean remove example "cmake ... -DTBB_DIR ..."?

Yes, either remove this example or write in the lines above "run cmake with following options" and change the example to contain -DUSE_TBB:

cmake .. -DUSE_TBBB=1 -DTBB_DIR=<Path to Intel TBB>/cmake

[igchor]

Reviewable status: 5 of 21 files reviewed, 22 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 37 at r25 (raw file):

#### To build packages ####

I still see a newline here (line 37)

[vinser52]

Reviewable status: 5 of 21 files reviewed, 22 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 178 at r21 (raw file):

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

I think in this case you should cast the return value to some one type - probably cast MSC version to int and maybe add some assert to make sure there is no overflow.

Done. I think assert is not required because _BitScanReverse64 returns bit possition in 64-bit variable. So the return value definitely does not exceed int max value.

[vinser52]

Reviewable status: 5 of 21 files reviewed, 22 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 1806 at r20 (raw file):

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

Why you don;t check if rehash is required in this case?

In if branch we already checked this by atomically reading bool flag. If we came to the else than rehash not required.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2761 at r21 (raw file):

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

why not just *p != n? or did you mean p && *p != n?

I delete this method. We need persistent TLS to properly implement it.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2764 at r21 (raw file):

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

what it means that *p == 0?

I delete this method. We need persistent TLS to properly implement it.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2784 at r21 (raw file):

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

Please add comment simmilar to the one in erase() - that we acquire lock only to make sure no one else holds it.

I delete this method. We need persistent TLS to properly implement it.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2803 at r21 (raw file):

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

can't you just move all the code that is done only once (code after (if(!*p) {...}) outside the loop?

I delete this method. We need persistent TLS to properly implement it.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2105 at r23 (raw file):

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

I think it should be "return", not "returns" for doxygen to properly parse it

According to documentation both variants are possible. See http://www.doxygen.nl/manual/commands.html#cmdreturns

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 1827 at r24 (raw file):

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

what we jumped to the code in "else" and just before scoped_t::acquire() some other thread inserted some element and enabled new segment so that bucket now require rehahsing? Is this scenario even possible?

No. "rehash required" flag set only for new bucket, not for existing one.

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2781 at r24 (raw file):

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

So what is TODO here?

It is legacy from TBB.

[vinser52]

Reviewable status: 5 of 21 files reviewed, 22 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 37 at r25 (raw file):

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

I still see a newline here (line 37)

Done. Just pushed new version. Please check again.

[vinser52]

Reviewable status: 5 of 21 files reviewed, 22 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 56 at r24 (raw file):

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

Yes, either remove this example or write in the lines above "run cmake with following options" and change the example to contain -DUSE_TBB:

cmake .. -DUSE_TBBB=1 -DTBB_DIR=<Path to Intel TBB>/cmake

Done.

[igchor]

Reviewable status: 5 of 21 files reviewed, 13 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

README.md, line 56 at r24 (raw file):

Previously, vinser52 (Sergei Vinogradov) wrote…

Done.

Please also remove : at the end of the sentence.

[vinser52]

Reviewable status: 4 of 21 files reviewed, 13 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/persistent_pool_ptr.hpp, line 294 at r11 (raw file):

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

I agree, we should probably put it in detail.

Done.

---

include/libpmemobj++/experimental/persistent_pool_ptr.hpp, line 33 at r24 (raw file):

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

Please move persistent_poo_ptr.hpp to detail folder and namespace, in future we could replace it with self-relative pointer and we don;'t really need to expose this pointer to the user.

Done.

[igchor]

Could you remove from commit message:
"Add clang-format checks ..."
and
"Fix CPack issue ... "?

Those were already included in other commits.

Reviewable status: 4 of 21 files reviewed, 11 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

[vinser52]

Reviewable status: 4 of 21 files reviewed, 11 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 1646 at r24 (raw file):

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

Could you change the comments starting with "//" to "/**/" ? So that it would be consistent with other files.

Done.

[igchor]

Reviewed 4 of 7 files at r22, 5 of 12 files at r23, 2 of 6 files at r24, 2 of 4 files at r25, 2 of 3 files at r26.
Reviewable status: 19 of 21 files reviewed, 11 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2638 at r26 (raw file):

	 * Delete item by accessor
	 */
	bool exclude(const_accessor &item_accessor);

Please remove this (exclude function implementation is removed).

[vinser52]

Reviewable status: 18 of 21 files reviewed, 11 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)

---

include/libpmemobj++/experimental/concurrent_hash_map.hpp, line 2638 at r26 (raw file):

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

Please remove this (exclude function implementation is removed).

Done.

[igchor]

Reviewed 2 of 3 files at r27.
Reviewable status: 20 of 21 files reviewed, 10 unresolved discussions (waiting on @igchor, @lplewa, @marcinslusarz, and @vinser52)