Commit 9031ee87 authored by Igor Kushnir's avatar Igor Kushnir

Reimplement DUChainReferenceCounting: QMap => array

The old DUChainReferenceCounting implementation is incorrect in many
places. But my assertions and tests in
kdevelop/kdevelop!190 indicate
that the wrong code was simply never executed even with global rather
than thread_local data. All overlapping ranges were always completely
equal (both the start and the size). The buggy range merging branch was
never taken.

I haven't encountered more than 2 different ranges at a time with
thread_local duchainReferenceCounting. So there is no need for a QMap or
a boost::icl::interval_map in DUChainReferenceCounting class. A simple
unsorted array of ranges should be perfect. The array size of 3 should
be safe enough.

Use a simple statically allocated array rather than QVector or
std::vector to safely eliminate a long-standing intentional memory leak
(new QMap without a matching delete).

Add a second parameter - unsigned size - to
disableDUChainReferenceCounting() in order to support nested reference
counting enabling/disabling with equal start but different size values.
While I haven't encountered any different overlapping requests,
supporting them doesn't significantly complicate the code. So why not?

Clean up DUChainReferenceCounting code:
* remove almost all reinterpret_cast uses;
* improve const correctness;
* char* => std::byte*;
* remove "DUChainReferenceCounting" suffixes from member function names.

Even with DUChainReferenceCounting::maxIntervalCount = 2, this change
does not break any kdevelop, kdev-python, kdev-php, kdev-ruby, kdev-css,
kdev-upload, kdev-verapp, kdev-krazy2, kdev-valgrind or kdev-xdebug
tests. No other maintained KDevelop plugin includes referencecounting.h
even indirectly, that is, none of the remaining maintained plugins
includes duchain, serialization or language headers.

This implementation simplification substantially speeds up
BenchItemRepository::shouldDoReferenceCounting() with both data rows.
But for some reason it slightly slows down
BenchIndexedString::bench_destroy(). The following table compares
performance of the previous, current and considered alternative
implementations in the affected benchmarks. The numbers denote
milliseconds per iteration.

version\benchmark       qhash   create  destroy shouldDo(-) shouldDo(+)
previous commit         1.2     149     103     212         318
inline f-l static       1.2     149     107     106         196
count == 0 -> false     1.2     148     106     106         212
Q_LIKELY(count == 0)    1.2     147     107     106         282
extern variable         1.4     153     128     636         671
inline variable         1.2     149     100     318         388
* return count != 0;    1.1     148     102     106         106
* return false;         0.87    141      67      70         <test fails>

        Legend
Benchmarks:
qhash       - BenchIndexedString::bench_qhashIndexedString()
create      - BenchIndexedString::bench_create()
destroy     - BenchIndexedString::bench_destroy()
shouldDo(-) - BenchItemRepository::shouldDoReferenceCounting(disabled)
shouldDo(+) - BenchItemRepository::shouldDoReferenceCounting(enabled)

Versions:
previous commit         - the version just before this commit
inline f-l static       - this commit
count == 0 -> false     - this commit with `if (count==0) return false;`
                          inserted at the beginning of
                          DUChainReferenceCounting::shouldDo()
Q_LIKELY(count == 0)    - same as "count == 0 -> false", but with
                          `Q_LIKELY` added in: `Q_LIKELY(count==0)`
extern variable         - this commit, but with extern thread_local
                          variable in the header instead of instance()
inline variable         - this commit, but with inline thread_local
                          variable in the header instead of instance()
* <code>                - this commit with <code> as a one-line wrong
                          implementation of
                          DUChainReferenceCounting::shouldDo()

The versions marked with '*' are incorrect. The table includes them to
show how much DUChainReferenceCounting affects each benchmark's speed.

Removing `Q_DISABLE_COPY_MOVE(DUChainReferenceCounting)` and
`DUChainReferenceCounting() = default;` has no effect on the benchmarks.

The 52216a13 commit message contains the
results of these same benchmarks at other versions of the code.

Each number in this and the older commit message's table is the minimum,
not the average, value of milliseconds per iteration after at least 10
runs of each benchmark. These minimum values were very consistent,
repeated many times. Usually the minimum value was also the most
frequent value. So even the slightest differences in performance are
unlikely to be caused by random fluctuations. Though a few tiny
variations are so strange that they most likely *are* spurious.

The two "count == 0" optimization attempts slightly speed up creating
and destroying IndexedString but significantly slow down the synthetic
shouldDoReferenceCounting(enabled) benchmark.

The "inline variable" version somewhat speeds up destroying
IndexedString but dramatically slows down both data rows of the
synthetic shouldDoReferenceCounting() benchmark. This inconsistent
effect on performance is surprising. As is the fact that the
"extern variable" version is by far the slowest across the board...
parent cc27f42f
# fixme: this looks like an actual leak upstream, report, investigate and/or fix it there
leak:editorconfig_parse
leak:KDevelop::DUChainReferenceCounting::DUChainReferenceCounting
......@@ -19,6 +19,7 @@
#ifndef KDEVPLATFORM_APPENDEDLIST_H
#define KDEVPLATFORM_APPENDEDLIST_H
#include <QList>
#include <QMutex>
#include <QVector>
......
......@@ -135,12 +135,12 @@ void DUChainBase::makeDynamic()
if (!d_func()->m_dynamic) {
Q_ASSERT(d_func()->classId);
DUChainBaseData* newData = DUChainItemSystem::self().cloneData(*d_func());
enableDUChainReferenceCounting(d_ptr,
DUChainItemSystem::self().dynamicSize(*static_cast<DUChainBaseData*>(d_ptr)));
const auto referenceCountingSize = DUChainItemSystem::self().dynamicSize(*static_cast<DUChainBaseData*>(d_ptr));
enableDUChainReferenceCounting(d_ptr, referenceCountingSize);
//We don't delete the previous data, because it's embedded in the top-context when it isn't dynamic.
//However we do call the destructor, to keep semantic stuff like reference-counting within the data class working correctly.
KDevelop::DUChainItemSystem::self().callDestructor(static_cast<DUChainBaseData*>(d_ptr));
disableDUChainReferenceCounting(d_ptr);
disableDUChainReferenceCounting(d_ptr, referenceCountingSize);
d_ptr = newData;
Q_ASSERT(d_ptr);
Q_ASSERT(d_func()->m_dynamic);
......
......@@ -84,7 +84,7 @@ void saveDUChainItem(QVector<TopDUContextDynamicData::ArrayWithPosition>& data,
if (!isSharedDataItem) {
item.setData(&target);
}
disableDUChainReferenceCounting(data.back().array.data());
disableDUChainReferenceCounting(data.back().array.data(), data.back().array.size());
} else {
//Just copy the data into another place, expensive copy constructors are not needed
#if defined(__GNUC__) && !defined(__INTEL_COMPILER) && (((__GNUC__ * 100) + __GNUC_MINOR__) >= 800)
......
......@@ -1191,7 +1191,7 @@ void StringSetRepository::itemRemovedFromSets(uint index)
KDevelop::enableDUChainReferenceCounting(&string, sizeof(KDevelop::IndexedString));
string.~IndexedString(); //Call destructor with enabled reference-counting
KDevelop::disableDUChainReferenceCounting(&string);
KDevelop::disableDUChainReferenceCounting(&string, sizeof(KDevelop::IndexedString));
}
void StringSetRepository::itemAddedToSets(uint index)
......@@ -1204,6 +1204,6 @@ void StringSetRepository::itemAddedToSets(uint index)
KDevelop::enableDUChainReferenceCounting(data, sizeof(KDevelop::IndexedString));
new (data) KDevelop::IndexedString(string); //Call constructor with enabled reference-counting
KDevelop::disableDUChainReferenceCounting(data);
KDevelop::disableDUChainReferenceCounting(data, sizeof(KDevelop::IndexedString));
}
}
......@@ -321,13 +321,14 @@ public:
Q_ASSERT(m_objectMap[localHash] == 0);
m_objectMap[localHash] = insertedAt;
const auto referenceCountingSize = dataSize();
if (markForReferenceCounting)
enableDUChainReferenceCounting(m_data, dataSize());
enableDUChainReferenceCounting(m_data, referenceCountingSize);
request.createItem(reinterpret_cast<Item*>(m_data + insertedAt));
if (markForReferenceCounting)
disableDUChainReferenceCounting(m_data);
disableDUChainReferenceCounting(m_data, referenceCountingSize);
return insertedAt;
}
......@@ -436,13 +437,14 @@ public:
#endif
//Last thing we do, because createItem may recursively do even more transformation of the repository
const auto referenceCountingSize = dataSize();
if (markForReferenceCounting)
enableDUChainReferenceCounting(m_data, dataSize());
enableDUChainReferenceCounting(m_data, referenceCountingSize);
request.createItem(reinterpret_cast<Item*>(m_data + insertedAt));
if (markForReferenceCounting)
disableDUChainReferenceCounting(m_data);
disableDUChainReferenceCounting(m_data, referenceCountingSize);
#ifdef DEBUG_CREATEITEM_EXTENTS
if (m_available >= 8) {
......@@ -559,13 +561,14 @@ public:
Item* item = const_cast<Item*>(itemFromIndex(index));
const auto referenceCountingSize = dataSize();
if (markForReferenceCounting)
enableDUChainReferenceCounting(m_data, dataSize());
enableDUChainReferenceCounting(m_data, referenceCountingSize);
ItemRequest::destroy(item, repository);
if (markForReferenceCounting)
disableDUChainReferenceCounting(m_data);
disableDUChainReferenceCounting(m_data, referenceCountingSize);
#ifndef QT_NO_DEBUG
#if defined(__GNUC__) && !defined(__INTEL_COMPILER) && (((__GNUC__ * 100) + __GNUC_MINOR__) >= 800)
......@@ -1047,9 +1050,10 @@ class DynamicItem
public:
DynamicItem(Item* i, void* start, uint size) : m_item(i)
, m_start(start)
, m_size{size}
{
if (markForReferenceCounting)
enableDUChainReferenceCounting(m_start, size);
enableDUChainReferenceCounting(m_start, m_size);
// qDebug() << "enabling" << i << "to" << (void*)(((char*)i)+size);
}
......@@ -1058,7 +1062,7 @@ public:
if (m_start) {
// qDebug() << "destructor-disabling" << m_item;
if (markForReferenceCounting)
disableDUChainReferenceCounting(m_start);
disableDUChainReferenceCounting(m_start, m_size);
}
}
......@@ -1081,6 +1085,7 @@ public:
private:
mutable void* m_start;
const unsigned m_size;
};
///@tparam Item See ExampleItem
......
......@@ -2,6 +2,7 @@
* This file is part of KDevelop
*
* Copyright 2009 David Nolden <david.nolden.kdevelop@art-master.de>
* Copyright 2020 Igor Kushnir <igorkuo@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU Library General Public License as
......@@ -21,128 +22,71 @@
#include "referencecounting.h"
#include "itemrepository.h"
#include "debug.h"
#include <QMap>
#include <QAtomicInt>
#include <QThread>
#include <algorithm>
namespace KDevelop {
void DUChainReferenceCounting::Interval::assign(Pointer newStart, unsigned newSize) noexcept
{
start = newStart;
size = newSize;
refCount = 1;
}
DUChainReferenceCounting::DUChainReferenceCounting()
: doReferenceCounting{false}
, refCountingHasAdditionalRanges{false} //Whether 'refCountingRanges' is non-empty
, refCountingRanges{new QMap<void*, QPair<uint, uint>>} //ptr, <size, count>, leaked intentionally!
//Speedup: In most cases there is only exactly one reference-counted range active,
//so the first reference-counting range can be marked here.
, refCountingFirstRangeStart{nullptr}
, refCountingFirstRangeExtent{0u, 0u}
auto DUChainReferenceCounting::findInterval(Pointer start, unsigned size) noexcept -> Interval*
{
qCDebug(SERIALIZATION).nospace() << "Creating DUChainReferenceCounting(" << this << ") in " << QThread::currentThread();
return std::find_if(intervals, intervals + count, [start, size](Interval interval) {
return interval.start == start && interval.size == size;
});
}
void DUChainReferenceCounting::disableDUChainReferenceCounting(void* start)
void DUChainReferenceCounting::enable(Pointer start, unsigned size)
{
if (refCountingFirstRangeStart &&
reinterpret_cast<char*>(refCountingFirstRangeStart) <= reinterpret_cast<char*>(start) &&
reinterpret_cast<char*>(start) < reinterpret_cast<char*>(refCountingFirstRangeStart) + refCountingFirstRangeExtent.first) {
Q_ASSERT(refCountingFirstRangeExtent.second > 0);
--refCountingFirstRangeExtent.second;
if (refCountingFirstRangeExtent.second == 0) {
refCountingFirstRangeExtent = qMakePair<uint, uint>(0, 0);
refCountingFirstRangeStart = nullptr;
auto* const interval = findInterval(start, size);
if (interval == intervals + count) {
if (count == maxIntervalCount) {
qFatal("DUChainReferenceCounting interval count limit of %zu exceeded!", count);
}
} else if (refCountingHasAdditionalRanges) {
QMap<void*, QPair<uint, uint>>::iterator it = refCountingRanges->upperBound(start);
if (it != refCountingRanges->begin()) {
--it;
if (reinterpret_cast<char*>(it.key()) <= reinterpret_cast<char*>(start) &&
reinterpret_cast<char*>(start) < reinterpret_cast<char*>(it.key()) + it.value().first) {
//Contained
} else {
Q_ASSERT(0);
}
}
Q_ASSERT(it.value().second > 0);
--it.value().second;
if (it.value().second == 0)
refCountingRanges->erase(it);
refCountingHasAdditionalRanges = !refCountingRanges->isEmpty();
// "push_back"
Q_ASSERT(count < maxIntervalCount);
interval->assign(start, size);
++count;
} else {
Q_ASSERT(0);
Q_ASSERT(interval < intervals + count);
++interval->refCount;
}
if (!refCountingFirstRangeStart && !refCountingHasAdditionalRanges)
doReferenceCounting = false;
#ifdef TEST_REFERENCE_COUNTING
Q_ASSERT(shouldDo(start));
Q_ASSERT(shouldDo(start + size - 1));
#endif
}
void DUChainReferenceCounting::enableDUChainReferenceCounting(void* start, unsigned int size)
void DUChainReferenceCounting::disable(Pointer start, unsigned size)
{
doReferenceCounting = true;
if (refCountingFirstRangeStart &&
reinterpret_cast<char*>(refCountingFirstRangeStart) <= reinterpret_cast<char*>(start) &&
reinterpret_cast<char*>(start) < reinterpret_cast<char*>(refCountingFirstRangeStart) + refCountingFirstRangeExtent.first) {
//Increase the count for the first range
++refCountingFirstRangeExtent.second;
} else if (refCountingHasAdditionalRanges || refCountingFirstRangeStart) {
//There is additional ranges in the ranges-structure. Add any new ranges there as well.
QMap<void*, QPair<uint, uint>>::iterator it = refCountingRanges->upperBound(start);
if (it != refCountingRanges->begin()) {
--it;
if (reinterpret_cast<char*>(it.key()) <= reinterpret_cast<char*>(start) &&
reinterpret_cast<char*>(start) < reinterpret_cast<char*>(it.key()) + it.value().first) {
//Contained, count up
} else {
it = refCountingRanges->end(); //Insert own item
}
} else if (it != refCountingRanges->end() && it.key() > start) {
//The item is behind
it = refCountingRanges->end();
}
if (it == refCountingRanges->end()) {
QMap<void*, QPair<uint, uint>>::iterator inserted = refCountingRanges->insert(start, qMakePair(size, 1u));
//Merge following ranges
QMap<void*, QPair<uint, uint>>::iterator it = inserted;
++it;
while (it != refCountingRanges->end() && it.key() < reinterpret_cast<char*>(start) + size) {
inserted.value().second += it.value().second; //Accumulate count
if (reinterpret_cast<char*>(start) + size < reinterpret_cast<char*>(inserted.key()) + it.value().first) {
//Update end position
inserted.value().first = (reinterpret_cast<char*>(inserted.key()) + it.value().first) - reinterpret_cast<char*>(start);
}
it = refCountingRanges->erase(it);
}
} else {
++it.value().second;
if (it.value().first < size)
it.value().first = size;
}
auto* const interval = findInterval(start, size);
Q_ASSERT(interval < intervals + count);
refCountingHasAdditionalRanges = true;
if (interval->refCount == 1) {
// "erase" interval
std::move(interval + 1, intervals + count, interval);
--count;
} else {
refCountingFirstRangeStart = start;
refCountingFirstRangeExtent.first = size;
refCountingFirstRangeExtent.second = 1;
Q_ASSERT(interval->refCount > 1);
--interval->refCount;
}
Q_ASSERT(refCountingHasAdditionalRanges == (refCountingRanges && !refCountingRanges->isEmpty()));
#ifdef TEST_REFERENCE_COUNTING
Q_ASSERT(shouldDoDUChainReferenceCounting(start));
Q_ASSERT(shouldDoDUChainReferenceCounting(reinterpret_cast<char*>(start) + (size - 1)));
#endif
}
void enableDUChainReferenceCounting(void* start, unsigned int size)
void enableDUChainReferenceCounting(const void* start, unsigned size)
{
DUChainReferenceCounting::instance().enableDUChainReferenceCounting(start, size);
DUChainReferenceCounting::instance().enable(reinterpret_cast<DUChainReferenceCounting::Pointer>(start), size);
}
void disableDUChainReferenceCounting(void* start)
void disableDUChainReferenceCounting(const void* start, unsigned size)
{
DUChainReferenceCounting::instance().disableDUChainReferenceCounting(start);
DUChainReferenceCounting::instance().disable(reinterpret_cast<DUChainReferenceCounting::Pointer>(start), size);
}
}
......
......@@ -2,6 +2,7 @@
* This file is part of KDevelop
*
* Copyright 2009 David Nolden <david.nolden.kdevelop@art-master.de>
* Copyright 2020 Igor Kushnir <igorkuo@gmail.com>
*
* This program is free software; you can redistribute it and/or modify
* it under the terms of the GNU Library General Public License as
......@@ -23,10 +24,9 @@
#include "serializationexport.h"
#include <QMap>
#include <QPair>
#include <QtGlobal>
#include <utility>
#include <cstddef>
//When this is enabled, the duchain unloading is disabled as well, and you should start
//with a cleared ~/.kdevduchain
......@@ -37,11 +37,17 @@ namespace KDevelop {
///so the reference-counting code can be inlined.
class KDEVPLATFORMSERIALIZATION_EXPORT DUChainReferenceCounting
{
#if QT_VERSION >= QT_VERSION_CHECK(5, 13, 0)
Q_DISABLE_COPY_MOVE(DUChainReferenceCounting)
#else
Q_DISABLE_COPY(DUChainReferenceCounting)
#endif
public:
bool shouldDoDUChainReferenceCounting(void* item) const;
void enableDUChainReferenceCounting(void* start, unsigned int size);
void disableDUChainReferenceCounting(void* start);
using Pointer = const std::byte*;
bool shouldDo(Pointer item) const noexcept;
void enable(Pointer start, unsigned size);
void disable(Pointer start, unsigned size);
static DUChainReferenceCounting& instance()
{
......@@ -50,51 +56,43 @@ public:
}
private:
DUChainReferenceCounting();
DUChainReferenceCounting() = default;
bool shouldDoDUChainReferenceCountingInternal(void* item) const;
struct Interval {
Pointer start;
unsigned size;
unsigned refCount;
bool doReferenceCounting;
bool refCountingHasAdditionalRanges;
QMap<void*, QPair<uint, uint>>* const refCountingRanges;
void* refCountingFirstRangeStart;
QPair<uint, uint> refCountingFirstRangeExtent;
};
constexpr bool contains(Pointer item) const noexcept { return item >= start && item < start + size; }
void assign(Pointer newStart, unsigned newSize) noexcept;
};
KDEVPLATFORMSERIALIZATION_EXPORT void initReferenceCounting();
Interval* findInterval(Pointer start, unsigned size) noexcept;
inline bool shouldDoDUChainReferenceCounting(void* item)
{
return DUChainReferenceCounting::instance().shouldDoDUChainReferenceCounting(item);
}
// I have never encountered more than 2 intervals at a time during my tests.
// So the maximum interval count of 3 should be more than enough for every practical use.
static constexpr std::size_t maxIntervalCount = 3;
inline bool DUChainReferenceCounting::shouldDoDUChainReferenceCountingInternal(void* item) const
std::size_t count = 0;
Interval intervals[maxIntervalCount];
};
inline bool DUChainReferenceCounting::shouldDo(Pointer item) const noexcept
{
auto it = std::as_const(*refCountingRanges).upperBound(item);
if (it != refCountingRanges->constBegin()) {
--it;
return reinterpret_cast<char*>(it.key()) <= reinterpret_cast<char*>(item) &&
reinterpret_cast<char*>(item) < reinterpret_cast<char*>(it.key()) + it.value().first;
for (std::size_t i = 0; i != count; ++i) {
if (intervals[i].contains(item)) {
return true;
}
}
return false;
}
KDEVPLATFORMSERIALIZATION_EXPORT void initReferenceCounting();
///This is used by indexed items to decide whether they should do reference-counting
inline bool DUChainReferenceCounting::shouldDoDUChainReferenceCounting(void* item) const
inline bool shouldDoDUChainReferenceCounting(const void* item) noexcept
{
if (!doReferenceCounting) //Fast path, no place has been marked for reference counting, 99% of cases
return false;
if (refCountingFirstRangeStart &&
(reinterpret_cast<char*>(refCountingFirstRangeStart) <= reinterpret_cast<char*>(item)) &&
(reinterpret_cast<char*>(item) < reinterpret_cast<char*>(refCountingFirstRangeStart) + refCountingFirstRangeExtent.first))
return true;
if (refCountingHasAdditionalRanges)
return shouldDoDUChainReferenceCountingInternal(item);
else
return false;
return DUChainReferenceCounting::instance().shouldDo(reinterpret_cast<DUChainReferenceCounting::Pointer>(item));
}
///Enable reference-counting for the given range
......@@ -105,11 +103,12 @@ inline bool DUChainReferenceCounting::shouldDoDUChainReferenceCounting(void* ite
///not care about this stuff at all.
///@param start Position where to start the reference-counting
///@param size Size of the area in bytes
KDEVPLATFORMSERIALIZATION_EXPORT void enableDUChainReferenceCounting(void* start, unsigned int size);
KDEVPLATFORMSERIALIZATION_EXPORT void enableDUChainReferenceCounting(const void* start, unsigned size);
///Must be called as often as enableDUChainReferenceCounting, with the same ranges
///Must never be called for the same range twice, and not for overlapping ranges
///@param start Position where the reference-counting was started
KDEVPLATFORMSERIALIZATION_EXPORT void disableDUChainReferenceCounting(void* start);
///@param size Size of the area where the reference-counting was started in bytes
KDEVPLATFORMSERIALIZATION_EXPORT void disableDUChainReferenceCounting(const void* start, unsigned size);
///Use this as local variable within the object that maintains the reference-count,
///and use
......
......@@ -246,9 +246,11 @@ void BenchItemRepository::shouldDoReferenceCounting()
constexpr std::size_t valueCount{100'000'000};
std::vector<Type> values(valueCount);
const auto referenceCountingStart = &values.front();
constexpr auto referenceCountingSize = valueCount * sizeof(Type);
QFETCH(bool, enableReferenceCounting);
if (enableReferenceCounting) {
enableDUChainReferenceCounting(&values.front(), values.size() * sizeof(Type));
enableDUChainReferenceCounting(referenceCountingStart, referenceCountingSize);
}
// NOTE: switching CountType from int to std::size_t slows down the "disabled" benchmark
......@@ -263,7 +265,7 @@ void BenchItemRepository::shouldDoReferenceCounting()
}
if (enableReferenceCounting) {
disableDUChainReferenceCounting(&values.front());
disableDUChainReferenceCounting(referenceCountingStart, referenceCountingSize);
QCOMPARE(count, static_cast<CountType>(values.size()));
} else {
QCOMPARE(count, 0);
......
Markdown is supported
0% or .
You are about to add 0 people to the discussion. Proceed with caution.
Finish editing this message first!
Please register or to comment