QMap: don't detach in take()

When shared, the old use used detach(), thereby copying the key (and
must needs allocating a node for it) just to extract it again in the
next step. While this is easy to understand, it has several problems:

First, we allocate a node for the to-be-removed item just to remove it
again (deallocating the node again). This is inefficient.

Second, because on detach(), we drop our own reference to the old data
early on in the function, a separate thread can drop its reference and
delete the old data while we still execute and reference 'key', which
may be an alias to a key in the old data. This was known and we
protected against it by taking _an extra copy_ of *this to retain the
reference to the old data for the remainder of the function. This is
also inefficient, because, while we anyway deep-copy, the ref-count
manipulations, being atomic, may actually dominate the runtime for
small maps.

So, as described in general in the detach-remove epic (QTBUG-103329),
build a new container, and swap() it into place. This solves both
problems while minimizing overhead.

Not picking to 6.8 and earlier, as we don't need to (optimization) and
the diff is very localized, so shouldn't affect cherry-picking of fixes
in adjacent lines.

Task-number: QTBUG-106170
Pick-to: 6.11 6.10
Change-Id: Ia9a9e72c85398e85c963c309b42d94482a4b6759
Reviewed-by: Allan Sandfeld Jensen <allan.jensen@qt.io>

Author: marcmutz

src/corelib/tools/qmap.h

@@ -12,6 +12,7 @@
 #include <QtCore/qlist.h>
 #include <QtCore/qrefcount.h>
 #include <QtCore/qpair.h>
+#include <QtCore/qscopeguard.h>
 #include <QtCore/qshareddata.h>
 #include <QtCore/qshareddata_impl.h>
 #include <QtCore/qttypetraits.h>
@@ -328,10 +329,45 @@ class QMap
         if (!d)
             return T();
 
-        const auto copy = d.isShared() ? *this : QMap(); // keep `key` alive across the detach
-        // TODO: improve. There is no need of copying all the
-        // elements (the one to be removed can be skipped).
-        detach();
+        if (d.isShared()) {
+            Map m;
+            // For historic reasons, we always un-share (was: detach()) when
+            // this function is called, even if `key` isn't found
+            const auto commit = qScopeGuard([&] { QMap{std::move(m)}.swap(*this); });
+
+            // This way of copying ought to be O(N) (not NlogN) and not causing
+            // any rebalancings in `m`, because we build in-order and with hint
+            // [[citation needed]].
+
+            const auto keep = [&m] (auto it) { m.insert(m.cend(), *it); };
+
+            auto it = d->m.cbegin();
+            const auto end = d->m.cend();
+            const auto cmp = d->m.key_comp();
+            while (it != end) {
+                if (cmp(it->first, key)) { // still before
+                    keep(it);
+                    ++it;
+                } else if (cmp(key, it->first)) { // after, iow: not found
+                    // This should be faster than an actual range-insert, because
+                    // the latter cannot assume that the input is sorted; we can:
+                    while (it != end) {
+                        keep(it);
+                        ++it;
+                    }
+                    break;
+                } else { // found!
+                    return [&] {
+                        T r = it->second; // we cannot move (isShared()!)
+                        while (++it != end)
+                            keep(it);
+                        return r;
+                    }();
+                }
+            }
+            // if we reach here, `key` wasn't found:
+            return T();
+        }
 
 #ifdef __cpp_lib_node_extract
         if (const auto node = d->m.extract(key))