Optimize BTreeMap::append() using CursorMut

[asder8215]

Since BTreeMap::merge uses CursorMut to avoid reconstructing the map from scratch and instead inserting other BTreeMap at the right places or overwriting the value in self BTreeMap on conflict, we might as well do the same for BTreeMap::append. This also means that some of the code in append.rs can be removed; bulk_push() however is used by bulk_build_from_sorted_iterator(), which is used by the From/FromIterator trait impl on BTreeMap. Feels like we should rename the file or place the bulk_push() in an existing file.

The same additional optimization consideration that BTreeMap::merge has is also applied to BTreeMap::append.

r? @Mark-Simulacrum since Mark has seen the BTreeMap::merge code already (only diff is the Ordering::Equal case and now one of the test assertions on a panic case has the correct value now).

[asder8215]

I'd be curious to see if this shows any noticeable performance difference with the current implementation of BTreeMap::append

[Mark-Simulacrum]

Do we think this is worth it vs. calling merge with the right conflict function?

[asder8215]

I guess it's a bit redundant with how merge does everything that append does with the added benefit on what you want the value to be on conflicting keys. I'm not sure why one would prefer having less flexibility deciding what the resultant value of conflicting keys should be with BTreeMap::append over using BTreeMap::merge.

I think I wanted the code internally for BTreeMap::append() to be consistent with BTreeMap::merge() since we can remove some code that only BTreeMap::append uses if we have BTreeMap::merge() and BTreeMap::append() sharing the same CursorMut functions. Moreover, I think I just found it unnecessary how the BTreeMap in BTreeMap::append constructs it from scratch rather than just placing the elements of other BTreeMap into the appropriate spots of self BTreeMap.

[Mark-Simulacrum]

Sorry, maybe just not understanding... my question is essentially: it seems like merge and append both have the same signature (BTreeMap + BTreeMap -> BTreeMap), right? So is there a reason we can't re-use the underlying implementation and only have one?

(If the answer is "that's slower because of the custom comparator", that seems like an OK answer, but also an unfortunate one. It doesn't sound like that's the answer you're giving though?)

[asder8215]

Oh sorry I think I misunderstood. If I'm understanding what you're saying here, you're asking why we can't use the same implementation inline from BTreeMap::merge to BTreeMap::append?

To clarify, BTreeMap::merge and BTreeMap::append do not have the same signature. BTreeMap::append's other takes in a &mut BTreeMap while BTreeMap::merge's other takes in an owned BTreeMap. This means that if I do other.into_iter() directly, iterating through this returns me (&K, &mut V) pairs, which we can't insert that into self BTreeMap since CursorMut's insert_* methods requires owned K and V values (and on conflict where we override the value of the conflicting key in self BTreeMap's, we still need a V to directly assign it to value in self's conflicting key entry).

The method signature for BTreeMap::append (with other taking a mutable borrow of BTreeMap) was actually acknowledged as a mistake within the ACP for BTreeMap::merge.

[Mark-Simulacrum]

If I'm understanding what you're saying here, you're asking why we can't use the same implementation inline from BTreeMap::merge to BTreeMap::append?

Yes, that's right.

To clarify, BTreeMap::merge and BTreeMap::append do not have the same signature.

The user-facing signature is different, but it seems like the &mut Map can be moved out of (replacing it with an empty map during iteration), right? That would give you an identical signature AFAICT, and is consistent with what we end up doing (draining the appended-from map fully).

Essentially this (playground):

#![feature(btree_merge)]
use std::collections::BTreeMap;

fn append<K, V>(this: &mut BTreeMap<K, V>, other: &mut BTreeMap<K, V>)
where
    K: Ord,
{
    // append uses other exclusively.
    this.merge(std::mem::take(other), |key, _this_value, other_value| {
        other_value
    });
}

[asder8215]

That's true, I forgot that std::mem::take can be used here to take &mut BTreeMap and replace it with the default empty BTreeMap. I'll update the code right now.

[asder8215]

Ah, there's an issue here, Default trait is implemented for BTreeMap<K, V>:

#[stable(feature = "rust1", since = "1.0.0")]
impl<K, V> Default for BTreeMap<K, V> {
    /// Creates an empty `BTreeMap`.
    fn default() -> BTreeMap<K, V> {
        BTreeMap::new()
    }
}

However, Default isn't implemented for BTreeMap<K, V, A>. The full compiler error I'm getting is saying:

error[E0277]: the trait bound `map::BTreeMap<K, V, A>: Default` is not satisfied
    --> library/alloc/src/collections/btree/map.rs:1308:30
     |
1308 |         self.merge(mem::take(other), |_key, _self_val, other_val|{
     |                    --------- ^^^^^ unsatisfied trait bound
     |                    |
     |                    required by a bound introduced by this call
     |
help: the trait `Default` is not implemented for `map::BTreeMap<K, V, A>`
    --> library/alloc/src/collections/btree/map.rs:189:1
     |
 189 | / pub struct BTreeMap<
 190 | |     K,
 191 | |     V,
 192 | |     #[unstable(feature = "allocator_api", issue = "32838")] A: Allocator + Clone = Global,
 193 | | > {
     | |_^
note: required by a bound in `core::mem::take`
    --> library/core/src/mem/mod.rs:820:22
     |
 820 | pub const fn take<T: [const] Default>(dest: &mut T) -> T {
     |                      ^^^^^^^^^^^^^^^ required by this bound in `take`
help: consider extending the `where` clause, but there might be an alternative better way to express this requirement
     |
1219 |         A: Clone, map::BTreeMap<K, V, A>: Default
     |                   +++++++++++++++++++++++++++++++
help: consider introducing a `where` clause, but there might be an alternative better way to express this requirement
     |
 701 | impl<K, V, A: Allocator + Clone> BTreeMap<K, V, A> where map::BTreeMap<K, V, A>: Default {

I'm not certain what else can be done here for BTreeMap::append since it's stabilized and I don't know if introducing a Default trait bound here for BTreeMap in this specific function would be a breaking change or acceptable change (also to note that BTreeSet::append uses BTreeMap::append as well).

[Mark-Simulacrum]

We already have A: Clone on append, so you should be able to move out of the map manually (replace the root node and then construct a new map with a cloned allocator, roughly). I think that code pattern is pre-existing at the top of the inner functions merge and append delegate to?

[asder8215]

Okay, I used mem::replace on other to take the content and place an empty BTreeMap inside. This worked for me.