[PATCH] D48896: [libcxx][c++17] P0083R5: Splicing Maps and Sets Part 2: merge
Erik Pilkington via Phabricator
reviews at reviews.llvm.org
Tue Oct 30 23:30:33 PDT 2018
erik.pilkington added a comment.
In https://reviews.llvm.org/D48896#1274970, @ldionne wrote:
> In https://reviews.llvm.org/D48896#1274938, @rsmith wrote:
>
> > In https://reviews.llvm.org/D48896#1274919, @rsmith wrote:
> >
> > > We can only do that if the comparators divide elements into the same equivalence classes, which we cannot know in general. (We can do this optimization if we can prove the comparator is a pure stateless function, but we probably can't detect that -- even an empty class type could store its state outside the class.) However, if the source and destination have the same comparator, and that comparator is `std::less<T>`, then I think it's correct to do this optimization.
> >
> >
> > Hmm, actually, we can probably do this in more cases than that. If we use the *destination's* comparator to check for equivalent elements instead of the source's comparator (under the assumption that the destination order is usually the same as the source order), this would work.
>
>
> Using the destination's comparator to perform this optimization is clever, and I believe it would work. This is much better than using the source's comparator when `std::is_empty<SourceComparator>` and `std::is_same<SourceComparator, DestinationComparator>` are both true.
>
> > But more generally, can we just provide the location of the most-recently-inserted element as an insertion hint and get this optimization as a special case of a more general optimization? But maybe that's actually what you were suggesting (the difference is hidden behind the "key in the source is not equivalent to the [other] key" comments -- equivalent according to which comparator?).
>
> The insertion hint idea did cross my mind when I was thinking about this but I did not pursue it further. I think it may be a cleaner way to achieve what I'm suggesting.
I think the insertion hint idea is really slick. I tried it out, and it does provide some pretty significant improvements, particularly when merging multi-containers that have a lot of duplicate keys. I would probably prefer doing it in a follow-up though, rather than complicating this "just get it working naively" patch.
================
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)
----------------
ldionne wrote:
> Can you add a quick comment that documents what this does, and what it returns?
Sure, sorry, I was just trying to conform to libcxx's "no comments" rule ;)
================
Comment at: libcxx/include/__hash_table:1940
+ __nd->__hash_ = hash_function()(__nd->__value_);
+ __next_pointer __ndptr =
+ __node_insert_unique_prepare(__nd->__hash(), __nd->__value_);
----------------
ldionne wrote:
> Consider using a more descriptive name for `__ndptr`.
Sure, new patch calls this `__existing_node`
================
Comment at: libcxx/include/__hash_table:2303
+ {
+ (void)__source.remove(__it++).release();
+ __src_ptr->__hash_ = __hash;
----------------
ldionne wrote:
> 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.
Sure, I agree that simplifies this. The new patch hoists the `++__it` out of the `if`.
================
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;
----------------
ldionne wrote:
> Same comment as above for `__it++`.
I don't think that `__source.remove(__it).release(); ++it;` works here because after `__source.remove(__it)`, `__it` is no longer a valid iterator, so you can't increment it.
================
Comment at: libcxx/include/__tree:2500
+ ++__i;
+ if (__child != nullptr)
+ continue;
----------------
ldionne wrote:
> 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?
>
> 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)?
Yep, exactly.
> 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?
`__child == __parent` and `__child != nullptr` should be equivalent. It looks like other users of __find_equal check `__child != nullptr`, why do you say that `__child == __parent` is "more correct"?
================
Comment at: libcxx/include/__tree:2502
+ continue;
+ __source.__remove_node_pointer(__src_ptr);
+ __insert_node_at(__parent, __child,
----------------
ldionne wrote:
> This doesn't throw, right? Would it make sense to mark `__remove_node_pointer` as `noexcept`? `__tree_remove` already is.
Yep, I agree. New patch adds _NOEXCEPT.
================
Comment at: libcxx/include/__tree:2557
+ __parent_pointer __parent;
+ __node_base_pointer& __child = __find_leaf_high(
+ __parent, _NodeTypes::__get_key(__src_ptr->__value_));
----------------
ldionne wrote:
> So this always returns a reference to the null leaf that should be replaced by the new node, correct?
Yep, thats my understanding as well.
================
Comment at: libcxx/include/__tree:2562
+ __insert_node_at(__parent, __child,
+ static_cast<__node_base_pointer>(__src_ptr));
+ }
----------------
ldionne wrote:
> 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.
>
Yep, I think you're analysis is right. Every (fancy) pointer conversion needs to go through `void`, but this doesn't. We actually do this a lot in this file. Do you think this actually matters in practice? Maybe @mclow.lists @EricWF have some thoughts?
================
Comment at: libcxx/include/map:946
+ _LIBCPP_INLINE_VISIBILITY __base& __get_tree() { return __tree_; }
+
----------------
ldionne wrote:
> Does this have to be public? Would doing otherwise require adding awkward friend declarations?
Yep, in order for `m1.merge(m2)` to work, someone needs access to both m1 and m2's `__tree_`s, which are currently private. On second thought, I think I'll just add the friend declarations, since it makes this a bit more explicit.
================
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);
+
----------------
ldionne wrote:
> 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.
Sure, done in the new patch.
https://reviews.llvm.org/D48896
More information about the libcxx-commits
mailing list