QDuplicateTracker: shed memory_resource on clear()

The pmr::monotonic_buffer_resource ("pmr::mbr") strategy can only
allocate buffers; deallocation is a no-op. I chose it for
QDuplicateTracker because the tool was only used one-off: build it,
then destroy it. There were no other operations.

That changed when 090c7e3262dce93df83c73a4a822761e5d9b4650 added
clear().

Calling pmr::unordered_set::clear() will deallocate the nodes (but not
the bucket array), but due to pmr::mbr, nothing is actually
deallocated, so memory that was freed by the container isn't actually
reused by it, later. IOW: repeated grow-clear-grow-clear cycles would
forever grow the monotonic_muffer_resource, using more and more memory
and thus appear to be leaking it.

This isn't exactly optimal behavior, so try to shed the pmr::mbr's
extra memory with a call to mbr::release().

Unfortunately, C++17 originally failed to nail down the semantics of
pmr::mbr::release(), prompting LWG 3120 in response to observed
implementation divergence. In particular, MSSTL, at the time of LWG
3120 filing, didn't appear to allow to allocate() after release(). In
our tests, it does allow it, it just never falls back to the original
(stack) buffer, and, looking at the implementation, it doesn't look
like it can do so anytime soon (it doesn't remember the buffer, and
everything is inline, so they would seem to need a BC break to fix it.

We tried to work-around the problem by hard-resetting 'res' by going
through a destroy-recreate cycle. pmr::mbr is neither copy- nor
move-constructible, so that blunt instrument is, unfortunately,
necessary (std::optional would be an alternative, but has overhead
that affects all platforms). This crashed, though, so just call
release() and handle MSSTL's failure to reuse the initial buffer as a
QoI issue.

For all platforms, before release(), we also need to make sure that
`set` no longer holds any references into it, and, since clear()
doesn't shed capacity, we need to use the C++11 version of the
swap-trick (swap is actually UB here, because the allocators differ):
move-assign a default-constructed set.

Amends 090c7e3262dce93df83c73a4a822761e5d9b4650.

Fixes: QTBUG-132945
Pick-to: 6.8 6.5 6.2
Change-Id: I4796806e427602255439dcb1518aa9b661c7933e
Reviewed-by: Thiago Macieira <thiago.macieira@intel.com>
(cherry picked from commit 9f4325e67354ce6c4c98e7a206f17729b378dc04)
Reviewed-by: Qt Cherry-pick Bot <cherrypick_bot@qt-project.org>
This commit is contained in:
Marc Mutz 2025-01-20 22:19:20 +01:00 committed by Qt Cherry-pick Bot
parent 605a20b262
commit d2c4f6df21
2 changed files with 24 additions and 8 deletions

View File

@ -45,7 +45,8 @@ class QDuplicateTracker {
char buffer[bufferSize(Prealloc)];
std::pmr::monotonic_buffer_resource res{buffer, sizeof buffer};
std::pmr::unordered_set<T, QHasher<T>> set{Prealloc, &res};
using Set = std::pmr::unordered_set<T, QHasher<T>>;
Set set{Prealloc, &res};
#else
class Set : public QSet<T> {
qsizetype setSize = 0;
@ -120,7 +121,20 @@ public:
void clear()
{
#ifdef __cpp_lib_memory_resource
// The birth defect of std::unordered_set is that both the nodes as
// well as the bucket array are allocated from the same allocator, so
// if we want to reclaim memory from the freed nodes, we also need to
// reclaim the memory for the bucket array.
set = Set(); // release all memory in `res` (clear() doesn't, and swap() is UB!)
res.release(); // restore to initial state (buffer, sizeof buffer)
// m_b_r can't reuse buffers, anyway
// now that `res` is reset to the initial state, also reset `set`:
set = Set{Prealloc, &res};
#else
set.clear();
#endif // __cpp_lib_memory_resource
}
};

View File

@ -96,13 +96,15 @@ void tst_QDuplicateTracker::clear()
QVERIFY(!tracker.hasSeen(1));
QVERIFY(tracker.hasSeen(1));
tracker.clear();
QVERIFY(!tracker.contains(0));
QVERIFY(!tracker.hasSeen(0));
QVERIFY(tracker.hasSeen(0));
QVERIFY(!tracker.hasSeen(1));
QVERIFY(tracker.hasSeen(1));
QVERIFY(tracker.contains(1));
for (int i = 0; i < 100; ++i) {
tracker.clear();
QVERIFY(!tracker.contains(0));
QVERIFY(!tracker.hasSeen(0));
QVERIFY(tracker.hasSeen(0));
QVERIFY(!tracker.hasSeen(1));
QVERIFY(tracker.hasSeen(1));
QVERIFY(tracker.contains(1));
}
}
void tst_QDuplicateTracker::appendTo()