[PATCH] D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge

[Louis Dionne via Phabricator]

ldionne requested changes to this revision.
ldionne added a comment.
This revision now requires changes to proceed.

When merging  a multi-container into a unique-container (e.g. `map.merge(multimap)` or `set.merge(multiset)`), I think we could do the following optimization: Once we've inserted an element from the `multimap` into the `map`, we could skip all the following equivalent elements in the `multimap` without having to perform a search in the `map` each time (since we know we won't insert the element anyway). Actually, I think we could do the same when we've found an element in the `map` and don't insert anything too. Basically something like this:

  template <class _Tp, class _Compare, class _Allocator>
  template <class _Tree>
  _LIBCPP_INLINE_VISIBILITY
  void
  __tree<_Tp, _Compare, _Allocator>::__node_handle_merge_unique(_Tree& __source)
  {
      static_assert(is_same<typename _Tree::__node_pointer, __node_pointer>::value, "");
  
      for (typename _Tree::iterator __i = __source.begin();
           __i != __source.end();)
      {
          __node_pointer __src_ptr = __i.__get_np();
          __parent_pointer __parent;
          __node_base_pointer& __child = __find_equal(
              __parent, _NodeTypes::__get_key(__src_ptr->__value_));
          ++__i;
          if (__child != nullptr) {
              // Here, increment __i until the key in the source is not equivalent to the key we found in *this.
              continue;
          } else {
              __source.__remove_node_pointer(__src_ptr);
              __insert_node_at(__parent, __child,
                               static_cast<__node_base_pointer>(__src_ptr));
              // Here, increment __i until the key in the source is not equivalent to the key we just inserted in *this.
          }
      }
  }

I guess the underlying idea is to move forward as much as possible in `__source` once we've put our hands on an element in `*this`, since we don't have to re-lookup in `*this` as long as the keys in `__source` are equivalent to the one we just processed. Do you think this optimization is (1) valid and (2) worth trying out?



================
Comment at: libcxx/include/__hash_table:1879
+typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::__next_pointer
+__hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_unique_prepare(
+    size_t __hash, value_type& __value)
----------------
Can you add a quick comment that documents what this does, and what it returns?


================
Comment at: libcxx/include/__hash_table:1910
+void
+__hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_unique_perform(
+    __node_pointer __nd) _NOEXCEPT
----------------
Ditto for documentation.


================
Comment at: libcxx/include/__hash_table:1940
+    __nd->__hash_ = hash_function()(__nd->__value_);
+    __next_pointer __ndptr =
+        __node_insert_unique_prepare(__nd->__hash(), __nd->__value_);
----------------
Consider using a more descriptive name for `__ndptr`.


================
Comment at: libcxx/include/__hash_table:1958
+typename __hash_table<_Tp, _Hash, _Equal, _Alloc>::__next_pointer
+__hash_table<_Tp, _Hash, _Equal, _Alloc>::__node_insert_multi_prepare(
+    size_t __cp_hash, value_type& __cp_val)
----------------
Ditto for documentation.


================
Comment at: libcxx/include/__hash_table:2303
+        {
+            (void)__source.remove(__it++).release();
+            __src_ptr->__hash_ = __hash;
----------------
Consider using `__source.remove(__it).release(); ++it;` instead -- it's a bit more straightforward. Also, I think you could hoist the `++__it` outside of the `if` altogether and get rid of the `else` branch.


================
Comment at: libcxx/include/__hash_table:2356
+            __node_insert_multi_prepare(__src_hash, __src_ptr->__value_);
+        (void)__source.remove(__it++).release();
+        __src_ptr->__hash_ = __src_hash;
----------------
Same comment as above for `__it++`.


================
Comment at: libcxx/include/__tree:2500
+        ++__i;
+        if (__child != nullptr)
+            continue;
----------------
Do I understand correctly that `__child != nullptr` if and only if the key is already in `*this`, in which case we `continue` to skip the insertion (because that is a container with unique keys)?

The comment in `__find_equal` says

```
// Find place to insert if __v doesn't exist
// Set __parent to parent of null leaf
// Return reference to null leaf
// If __v exists, set parent to node of __v and return reference to node of __v
```

It looks like the "more correct" check to see whether the key already exists in `*this` would have been ` if (__child == __parent)` -- is that correct?



================
Comment at: libcxx/include/__tree:2502
+            continue;
+        __source.__remove_node_pointer(__src_ptr);
+        __insert_node_at(__parent, __child,
----------------
This doesn't throw, right? Would it make sense to mark `__remove_node_pointer` as `noexcept`? `__tree_remove` already is.


================
Comment at: libcxx/include/__tree:2503
+        __source.__remove_node_pointer(__src_ptr);
+        __insert_node_at(__parent, __child,
+                         static_cast<__node_base_pointer>(__src_ptr));
----------------
This doesn't throw, right? Would it make sense to mark `__insert_node_at` as `noexcept`? `__tree_balance_after_insert` already is.


================
Comment at: libcxx/include/__tree:2557
+        __parent_pointer __parent;
+        __node_base_pointer& __child = __find_leaf_high(
+            __parent, _NodeTypes::__get_key(__src_ptr->__value_));
----------------
So this always returns a reference to the null leaf that should be replaced by the new node, correct?


================
Comment at: libcxx/include/__tree:2562
+        __insert_node_at(__parent, __child,
+                         static_cast<__node_base_pointer>(__src_ptr));
+    }
----------------
Regarding static casting from `__node_pointer` to `__node_base_pointer` -- is this actually guaranteed to work for fancy pointer types? I don't think it is -- I think only casting to `void_ptr` and then back to `__node_base_pointer` would work per the `Allocator` concept requirements.

Note that in practice, a fancy pointer that would not support that would be misbehaved. Also, this problem already exists elsewhere in the code. I'm asking just for curiosity, I don't expect you to act on this comment in any way.



================
Comment at: libcxx/include/map:946
 
+    _LIBCPP_INLINE_VISIBILITY __base& __get_tree() { return __tree_; }
+
----------------
Does this have to be public? Would doing otherwise require adding awkward friend declarations?


================
Comment at: libcxx/test/std/containers/associative/map/map.modifiers/merge.pass.cpp:17
+// template <class C2>
+//   void merge(map<key_type, C2, allocator_type>& source);
+
----------------
I think you mean `map<key_type, value_type, C2, allocator_type>& source);`. Also, you should mention the `&&` overload.

Can you also please add tests that perform `dst.merge(std::move(src))` before having done `dst.merge(src)` (the non-move version). Right now, we only seem to test that `dst.merge(std::move(src))` works when the operation is a no-op anyway.

This comment applies to the other containers as well.


https://reviews.llvm.org/D48896