GitLab

The recent commit that introduced Path::compare() and reimplemented
Path::operator< in terms of this new member function has unexpectedly
slowed down TestPath::bench_hash(), which never even calls compare() or
operator<, by 10%. This slowdown can be triggered by adding any
non-inline member function with a simple enough implementation to Path.
Inlining the now one-line Path::operator< restores the benchmark's
performance to the previous level.

The benefit of the inlining is the self-documentation of the
comparison's case sensitivity. The downside is that the implementation
can no longer be optimized without an ABI break. This downside is not a
problem for KDevPlatform, because each minor release breaks the ABI, and
optimizations don't target patch releases.

Assigning Path{} should be more efficient most of the time. The absence
of the clear() member function forces using the usually faster
alternative. QVector::clear() is very slow if the detaching happens,
because the elements are first copied, then destroyed. Assigning Path{}
is always extremely fast, because the default Path constructor is inline
and QVector's move assignment operator boils down to several raw pointer
assignments. The only situation where clear() could be faster is when
detaching does not happen and the QVector's capacity is reused. But this
scenario seems to be unlikely for Path.

Inlining the default constructor improves performance when a collection
is resized to a large number of default-constructed elements. This
happens when ProjectFileDataProvider::reset() is called for the first
time after a large project is opened. My benchmarking indicates that
this change speeds up the first call to ProjectFileDataProvider::reset()
by about 4%.

The documentation of this constructor already promises "an empty,
invalid Path", so making Path::m_data non-empty in the default
constructor would be an API break. Therefore a potential ABI break
because of the inline constructor shouldn't be of concern.

This must be more efficient.

The commit history is silent about the reason why folders and targets
are sorted case-sensitively, while files - case-insensitively. Sorting
all items case-insensitively is more consistent and potentially faster
as std::stable_sort used by QSortFilterProxyModel could start with a
closer-to-sorted range then.

- Also changed bitwidth to uint16_t otherwise is too big by 1 byte
  to reduce the aligned size

Previously we only supported this information for class declarations
and class members. But I find myself wanting to inspect this
information frequently for other types too, most notably type aliases
that instantiate a template.

Move the data from the ClassMemberDeclaration to the abstract type
and set it for all C++/C types we encounter that have that data.
Explicitly filter out function types, because we get strange data
there with libclang: sizeof 1, alignof 4? In reality one cannot
directly apply sizeof to a function anyways.

Also only unalias the types in the navigation context instead of
fully dereference them, as that would break the sizeof for pointers.

As this changes the layout of the on-disk DUChain data, we have to
bump our cache version number and trigger a full reparse.

Move the ProjectsGenerator to kdevplatform tests in order to make it accessible from other projects.

Use the now moved ProjectsGenerator to generate projects for testing in custom buildsystem plugin.
Clean up existing test-projects under git control.

This change does not affect performance of
DUChainReferenceCounting-sensitive benchmarks.

Clang 8.0 rejects this standard-compliant code with the following error
messages:
  1) default member initializer for 'count' needed within definition of
  enclosing class 'DUChainReferenceCounting' outside of member unctions
  2) <same as (1) but with 'intervals' in place of 'count'>

This change does not affect performance of
DUChainReferenceCounting-sensitive benchmarks.

This is pretty nasty, but at least it fixes the memory leak...
We now manually delete the ctest suites and kill the jobs when
we remove a project or update its data.

In practice reference counting is enabled on any interval only in small
code fragments. shouldDoDUChainReferenceCounting() is called only in
assertions (which are disabled in Release mode) and in places that lock
a mutex if it returns true. The performance penalty of a wrong
Q_UNLIKELY prediction should be small in comparison to the mutex lock.

Comparison of the generated assembly code with and without Q_UNLIKELY
indicates that this keyword helps the compiler to order the code in a
way optimized for the more common path. Specifically, in the case of
~IndexedString() (in which DUChainReferenceCounting::shouldDo() is
inlined), Q_UNLIKELY eliminates a jump from the fast path: a `jne`
instruction replaces `je` and the fast-path code is moved up.

The following table compares performance of the previous, current and
considered alternative implementations in the affected benchmarks. The
numbers denote milliseconds per iteration. These numbers are minimums,
not averages. The 9031ee87 commit
message specifies the meaning of the columns in the table legend and the
methodology of benchmarking in the paragraph that starts with
"Each number in this and the older commit message's table".

version\benchmark       qhash   create  destroy shouldDo(-) shouldDo(+)
previous commit         0.95    143      75      70         171
this commit             0.84    141      74      70         247
redundant if            0.84    141      74      70         247
inline variable         0.84    141      74      70         247
static data member      0.93    143      79     141         353
extern variable         0.93    143      79     141         353
internal linkage        1.1     145     100     565         742
non-inline f-l static   1.1     146      96     565         742
std::any_of             0.85    142      74      70         565

Versions:
redundant if            - a more verbose Q_UNLIKELY optimization:
                        if (Q_UNLIKELY(count != 0)) {
                             for (std::size_t i = 0; i != count; ++i) {
                                ...
                          Milian suggested to move Q_UNLIKELY into the
                          loop exit condition to eliminate the redundant
                          `if` - after I benchmarked all alternative
                          implementations with the `if`. The benchmark
                          performance and the generated assembly code on
                          the fast path remained the same.
inline variable         - "redundant if", but with inline thread_local
                          variable in the header instead of instance()
static data member      - "redundant if", but with instance as a static
                          data member, not a static member function
extern variable         - "redundant if", but with extern thread_local
                          variable in the header instead of instance()
internal linkage        - "redundant if", but with static thread_local
                          variable in the cpp file instead of instance()
                          and shouldDoDUChainReferenceCounting() defined
                          in the cpp file
non-inline f-l static   - "redundant if", but with instance() defined in
                          the cpp file
std::any_of             - "redundant if", but with shouldDo()
                          reimplemented as follows:
    if (Q_LIKELY(count == 0)) return false;
    return std::any_of(intervals, intervals + count,
        [item](Interval interval) { return interval.contains(item); });

Now that the constructor of DUChainReferenceCounting is constexpr, the
performance of the inline variable is exactly the same as of the inline
function with a function-local static variable inside. I think we should
keep the current version, because it would probably be faster if the
constructor becomes non-constexpr in the future. In addition, lazy
initialization of the thread_local instance variable is preferable and
should be enforced, because most threads never use
DUChainReferenceCounting.

Eliminating dynamic initialization of the function-local thread_local
DUChainReferenceCounting variable substantially improves performance of
shouldDoDUChainReferenceCounting(). The only really useful code change
is assigning {} to intervals. Explicitly specifying `constexpr` and
`noexcept` does not affect performance, but I added them anyway to
indicate that these properties improve the performance of this class.
I have tried replacing `= {}` with `{}`, removing Q_DISABLE_COPY_MOVE
along with the user-declared default constructor, but these tweaks did
not affect performance.

The following table compares performance of the previous, current and a
considered alternative implementation in the affected benchmarks. The
numbers denote milliseconds per iteration. These numbers are minimums,
not averages. The 9031ee87 commit
message specifies the meaning of the columns in the table legend and the
methodology of benchmarking in the paragraph that starts with
"Each number in this and the older commit message's table".

version\benchmark       qhash   create  destroy shouldDo(-) shouldDo(+)
previous commit         1.2     149     107     106         196
this commit             0.95    143      75      70         171
std::any_of             1.3     149     107     318         388

"std::any_of" is the version at this commit, but with
DUChainReferenceCounting::shouldDo() reimplemented as follows:
    return std::any_of(intervals, intervals + count,
        [item](Interval interval) { return interval.contains(item); });

I have also benchmarked passing `interval` by const reference in the
"std::any_of" version, which did not substantially affect performance
(only BenchIndexedString::bench_destroy() became slower: 107 => 110).
In a currently predominant 64-bit architecture, sizeof(Interval) == 16,
so Interval is passed by value across DUChainReferenceCounting code as
per C++ Core Guidelines (For "in" parameters, pass cheaply-copied types
by value and others by reference to const).

GIT_SILENT

A style may want to change the surface format of the window. Changing the
surface format after the window has been created has no effect though.
One example of this is Breeze where the context menus created here had an
ugly border instead of the intended look. This is because the setting of
Qt::WA_TranslucentBackground by breeze had no effect anymore after the
window was already created.

d->filesToParse is never used after all its elements are queued for
parsing. If parseAllProjectSources=true is passed to ParseProjectJob(),
this set contains almost as any elements as the number of files in the
project. If at least one of the project's documents is open, this set is
already detached, so setting it to {} frees memory. Otherwise, setting
to {} prevents detaching in case the project's file set changes later.

GIT_SILENT

* disable copying and moving of DynamicItem and rely on C++17's
  mandatory elision of copy/move operations;
* improve const correctness;
* make the constructor explicit (this should be the default);
* uint type alias => unsigned to match DUChainReferenceCounting's type;
* m_item: public => private.

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...

MSVC intentionally does not support DLL import/export of thread_local
variables. The result is the following compilation error:
C:\CI\Job Build\kdevplatform\serialization\referencecounting.h(57): error C2492:
'duchainReferenceCounting': data with thread storage duration may not have dll interface

In addition, this inline function-local static version unexpectedly
turned out to be much faster than the previous exported extern variable
version. The following table compares performance of the past, current
and considered alternative implementations in the affected benchmarks.
The numbers denote milliseconds per iteration.

version\benchmark       qhash   create  destroy shouldDo(-) shouldDo(+)
no thread_local         0.67    136     50      70          3146
bool thread_local       1.0     144     85      247         3294
exported thread_local   1.4     151     121     636         707
internal linkage        1.5     151     131     883         1025
non-inline f-l static   1.5     152     138     954         1060
inline variable         1.2     147     107     388         459
inline f-l static       1.2     149     103     212         318

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

Versions:
no thread_local         - 8fca91a0
bool thread_local       - db67d592
exported thread_local   - the version just before this commit
internal linkage        - static thread_local variable in the cpp file
non-inline f-l static   - instance() defined in the cpp file
inline variable         - inline thread_local variable in the header
inline f-l static       - this commit

Among other things this ensures that we don't load plugins that
we don't need in some of our tests, as we no longer check the
user's configuration for which plugin is enabled.

This fixes an UB cast when we call this on a ClassFunctionDeclaration
with setDeclarationIsDefinition(true) as shown in this UBSAN report:

```
/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/functiondefinition.cpp:93:49: runtime error: downcast of address 0x6060005c72c0 which does not point to an object of type 'FunctionDefinition'
0x6060005c72c0: note: object is of type 'KDevelop::ClassFunctionDeclaration'
 26 02 00 76  f0 06 b1 d3 05 7f 00 00  a0 64 21 00 b0 60 00 00  50 6e 22 00 20 60 00 00  10 2b 01 01
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'KDevelop::ClassFunctionDeclaration'
    #0 0x7f05d2c5377f in KDevelop::FunctionDefinition::definition(KDevelop::Declaration const*) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/functiondefinition.cpp:93
    #1 0x7f05dddae858 in accept /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/duchain/documentfinderhelpers.cpp:63
    #2 0x7f05d2f7e0c2 in KDevelop::DUChainUtils::collectItems(KDevelop::DUContext*, KDevelop::DUChainUtils::DUChainItemFilter&) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/duchainutils.cpp:422
    #3 0x7f05d2f7e4ed in KDevelop::DUChainUtils::collectItems(KDevelop::DUContext*, KDevelop::DUChainUtils::DUChainItemFilter&) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/duchainutils.cpp:432
    #4 0x7f05dddacb36 in duchainBuddyFile /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/duchain/documentfinderhelpers.cpp:136
    #5 0x7f05dddb2fcc in DocumentFinderHelpers::potentialBuddies(QUrl const&, bool) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/duchain/documentfinderhelpers.cpp:256
    #6 0x7f05dddb3a5d in DocumentFinderHelpers::sourceForHeader(QString const&) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/duchain/documentfinderhelpers.cpp:272
    #7 0x7f05dda0a7d5 in ClangRefactoring::moveIntoSource(KDevelop::IndexedDeclaration const&) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/codegen/clangrefactoring.cpp:158
    #8 0x563aae0570b3 in TestAssistants::testMoveIntoSource() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/tests/test_assistants.cpp:686
    #9 0x563aae01da1f in TestAssistants::qt_static_metacall(QObject*, QMetaObject::Call, int, void**) plugins/clang/tests/test_assistants_autogen/EWIEGA46WW/moc_test_assistants.cpp:127
    #10 0x7f05c7f8458a in QMetaMethod::invoke(QObject*, Qt::ConnectionType, QGenericReturnArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument, QGenericArgument) const (/usr/lib/libQt5Core.so.5+0x2c458a)
    #11 0x7f05df029e96  (/usr/lib/libQt5Test.so.5+0x18e96)
    #12 0x7f05df02a750  (/usr/lib/libQt5Test.so.5+0x19750)
    #13 0x7f05df02ad03  (/usr/lib/libQt5Test.so.5+0x19d03)
    #14 0x7f05df02b1cd in QTest::qRun() (/usr/lib/libQt5Test.so.5+0x1a1cd)
    #15 0x7f05df02b57d in QTest::qExec(QObject*, int, char**) (/usr/lib/libQt5Test.so.5+0x1a57d)
    #16 0x563aae03e824 in main /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/clang/tests/test_assistants.cpp:61
    #17 0x7f05c6e72151 in __libc_start_main (/usr/lib/libc.so.6+0x28151)
    #18 0x563aae01d1bd in _start (/home/milian/projects/kf5/build-dbg/extragear/kdevelop/kdevelop-sanitized/bin/test_assistants+0xe41bd)
```

Don't cast partially destroyed object, fixes UBSAN report:

```
/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/highlighting/codehighlighting.cpp:612:21: runtime error: downcast of address 0x608000240320 which does not point to an object of type 'DocumentChangeTracker'
0x608000240320: note: object is of type 'QObject'
 14 01 80 2f  e0 84 05 4b 5d 7f 00 00  a0 03 24 00 80 60 00 00  10 ee 11 00 20 60 00 00  01 be be be
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'QObject'
    #0 0x7f5d566f5920 in KDevelop::CodeHighlighting::trackerDestroyed(QObject*) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/highlighting/codehighlighting.cpp:612
```

Order isn't important for these maps, use the faster hash instead.

Only connect to qApp in init and disconnect in cleanup.
Then don't call cleanup in NoUi tests.

Fixes UBSAN report:
```
/home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/shell/uicontroller.cpp:241:16: runtime error: downcast of address 0x7f30c868a1c0 which does not point to an object of type 'QApplication'
0x7f30c868a1c0: note: object is of type 'QCoreApplication'
 00 00 00 00  f0 4f 59 de 30 7f 00 00  e0 55 02 00 e0 60 00 00  00 00 00 00 00 00 00 00  00 00 00 00
              ^~~~~~~~~~~~~~~~~~~~~~~
              vptr for 'QCoreApplication'
    #0 0x7f30eeacada3 in KDevelop::UiController::~UiController() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/shell/uicontroller.cpp:241
    #1 0x7f30eeacb874 in KDevelop::UiController::~UiController() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/shell/uicontroller.cpp:242
    #2 0x7f30eea745d3 in KDevelop::CorePrivate::~CorePrivate() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/shell/core.cpp:279
    #3 0x7f30eea7f4be in KDevelop::Core::~Core() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/shell/core.cpp:330
    #4 0x7f30f1250435 in KDevelop::TestCore::~TestCore() kdevplatform/tests/KDevPlatformTests_autogen/EWIEGA46WW/../../../../../../../../src/extragear/kdevelop/kdevelop/kdevplatform/tests/testcore.h:75
    #5 0x7f30f1250435 in KDevelop::TestCore::~TestCore() kdevplatform/tests/KDevPlatformTests_autogen/EWIEGA46WW/../../../../../../../../src/extragear/kdevelop/kdevelop/kdevplatform/tests/testcore.h:75
    #6 0x7f30de32a33f in QObject::event(QEvent*) (/usr/lib/libQt5Core.so.5+0x2e233f)
    #7 0x7f30de2fda4f in QCoreApplication::notifyInternal2(QObject*, QEvent*) (/usr/lib/libQt5Core.so.5+0x2b5a4f)
    #8 0x7f30de300572 in QCoreApplicationPrivate::sendPostedEvents(QObject*, int, QThreadData*) (/usr/lib/libQt5Core.so.5+0x2b8572)
    #9 0x7f30de3522dd in QTest::qWait(int) (/usr/lib/libQt5Core.so.5+0x30a2dd)
    #10 0x7f30f1279cef in KDevelop::TestCore::shutdown() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/tests/testcore.cpp:99
```

GIT_SILENT

GIT_SILENT

GIT_SILENT

GIT_SILENT

Move the variables into a simple new DUChainReferenceCounting class to
avoid the repetition of the `thread_local` keyword.

Reorder the declarations and initializations of two variables -
refCountingHasAdditionalRanges and refCountingRanges - to minimize
sizeof(DUChainReferenceCounting) while preserving the logic of the
variable ordering and grouping within the class.

Eliminate the mutex and the redundant dependency between threads.

This should work perfectly if the reference counting of each object is
confined to a single thread. This will break code that calls
DUChainReferenceCounting functions on the same object from multiple
threads. Hopefully such code does not exist.

This change does not break any kdevelop or kdev-python tests.