GitLab

In case of conflict in i18n, keep the version of the branch "ours"
To resolve a particular conflict, "git checkout --ours path/to/file.desktop"

Morten Volden authored Nov 09, 2020 and Milian Wolff committed Nov 09, 2020

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

Kai Stierand authored Oct 24, 2020 and Milian Wolff committed Nov 04, 2020

A path may now start either with 'X:\' or '/'

Kai Stierand authored Oct 24, 2020 and Milian Wolff committed Nov 04, 2020

Single quotes don't work on Windows.

Kai Stierand authored Oct 24, 2020 and Milian Wolff committed Nov 04, 2020

This is particularly necessary for Windows, where we have
clang-tidy in 'C:\Program Files\LLVM\bin' on a default installation.

Most notably, don't use a heap-allocated QListIterator, that's just
insane. Use a plain int to index into a vector, which is much much
more efficient.

Also simplify the test to ensure it properly awaits the load-or-error
case properly.

The qmake process is run asynchronously, so we have to wait for
the data in the test, otherwise it will just skip right through
most of it...

This actually shows that we are destroying the plugin repeatedly
before the process finishes, which means the slot gets disconnected
and never delets the process...

But really, this is just a stop-gap measure to
prevent LSAN reports in the unit test. We still
basically leak the deleted breakpoints until we
destroy the model, so nothing really is won here.
But it seems like the whole ownership and lifetime
tracking of breakpoints is utterly broken. Note how
we store e.g. raw pointers in the breakpoint controller
and don't seem to clean those up ever properly either...

I often just set CMAKE_CXX_DEBUG_FLAGS to contain
`-fsanitize=address,undefined` which would then influence the
observed behavior of the debuggees in test_gdb and lead to
failed tests.

Instead, explicitly disable the sanitizers to make sure we don't
run into this corner case.

Fixes LSAN report:

```
Indirect leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7fbbce16ef41 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x56469f8deab5 in KDevMI::GDB::DebugSession::execInferior(KDevelop::ILaunchConfiguration*, IExecutePlugin*, QString const&) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/gdb/debugsession.cpp:217
    #2 0x56469fa482f2 in KDevMI::MIDebugSession::startDebugging(KDevelop::ILaunchConfiguration*, IExecutePlugin*) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/debuggercommon/midebugsession.cpp:284
    #3 0x56469f81ef97 in KDevMI::GDB::GdbTest::testRemoteDebug() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/gdb/unittests/test_gdb.cpp:1658
```

Because the DUChainBaseData doesn't have a vtable yet is used
polymorphically in the DUChain, we have to make sure to call
delete on the correct type.

Generally, instead of manually calling the destructor of the
concrete type and then deleting the base type, we should just
call delete on the concrete type directly.

Fixes ASAN report:
```
==72034==ERROR: AddressSanitizer: new-delete-type-mismatch on 0x606000359de0 in thread T0:
  object passed to delete has wrong type:
  size of the allocated type:   56 bytes;
  size of the deallocated type: 20 bytes.
    #0 0x7f0393be7009 in operator delete(void*, unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:172
    #1 0x7f038a46985d in KDevelop::DUChainBase::~DUChainBase() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/duchainbase.cpp:98
    #2 0x7f038a10b691 in KDevelop::DUContext::~DUContext() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/ducontext.cpp:431
    #3 0x7f038a10c68e in KDevelop::DUContext::~DUContext() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/ducontext.cpp:477
    #4 0x7f038a13855b in void qDeleteAll<QTypedArrayData<KDevelop::DUContext*>::const_iterator>(QTypedArrayData<KDevelop::DUContext*>::const_iterator, QTypedArrayData<KDevelop::DUContext*>::const_iterator) /usr/include/qt/QtCore/qalgorithms.h:320
    #5 0x7f038a0a5d0f in void qDeleteAll<QVector<KDevelop::DUContext*> >(QVector<KDevelop::DUContext*> const&) /usr/include/qt/QtCore/qalgorithms.h:328
    #6 0x7f038a0a5d0f in KDevelop::DUContext::deleteChildContextsRecursively() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/ducontext.cpp:1067
    #7 0x7f038a20094c in KDevelop::TopDUContext::~TopDUContext() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/topducontext.cpp:586
    #8 0x7f038a200f72 in KDevelop::TopDUContext::~TopDUContext() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/topducontext.cpp:589
    #9 0x7f038a1f3b23 in KDevelop::TopDUContext::deleteSelf() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/topducontext.cpp:599
    #10 0x7f038a0626b5 in KDevelop::DUChainPrivate::removeDocumentChainFromMemory(KDevelop::TopDUContext*) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/duchain.cpp:456
    #11 0x7f0389f49264 in KDevelop::DUChain::removeDocumentChain(KDevelop::TopDUContext*) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/duchain.cpp:1312
    #12 0x55ea5065b6fa in TestContext::~TestContext() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/tests/test_duchain.cpp:454
    #13 0x55ea505e5462 in TestDUChain::testImportStructure() /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/tests/test_duchain.cpp:651

0x606000359de0 is located 0 bytes inside of 56-byte region [0x606000359de0,0x606000359e18)
allocated by thread T0 here:
    #0 0x7f0393be5f41 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x7f038a0dedb1 in KDevelop::DUContext::DUContext(KDevelop::RangeInRevision const&, KDevelop::DUContext*, bool) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/language/duchain/ducontext.cpp:357
```

Don't copy Path segments unnecessarily in qHash(const Path&). This
function may be called often in performance-critical code.

This commit speeds up BenchQuickOpen::benchProjectFileFilter_setFilter
by about 10% when the filter is not empty. matchPathFilter() calls
Path::segments() twice, is called by PathFilter::setFilter() in a loop.

I have examined all usages of Path::segments() in KDevelop code and
found no possibility of a regression because of this return type change.
There are no usages of Path::segments() in maintained KDevelop plugins.

In many usages copying the QVector is unnecessary and it doesn't occur
with this change, which improves performance. Copying a QVector could be
the bottleneck in some cases. One such case was a lambda in
ProjectFileDataProvider::projectClosing(), which called this function on
each file's projectPath (in an optimization now discarded in favor of an
almost equivalent Path::operator== change in the previous commit).

Arguments for returning the QVector by value:
  * returning objects by value is generally considered safer than
    returning them by reference;
  * returning by value gives more freedom to change the implementation
    of a class without changing its interface;
  * non-template Qt classes conventionally return by value relying on
    copy-on-write to make this relatively cheap.

Arguments for returning the QVector by reference:
  * Returning by reference is not very dangerous. This pattern is not
    uncommon in non-COW C++ code and is used extensively by Qt and std
    container class templates, so developers know what to expect and how
    to use such interfaces safely. If callers of this function store the
    return value in a reference variable for optimization, they are
    responsible for ensuring the safety of such usage. If they simply
    use segments() in an expression or assign its return value to a
    non-reference variable (e.g. with `auto`), then they should be safe
    from reference-related bugs.
  * Path is a class whose main purpose is optimization. Its layout is
    already inline. If it switches to some other class for storing its
    internal data, the ABI change is unavoidable; the not so many usages
    of segments() could be adjusted accordingly rather than become
    silently a lot less efficient.
  * Returning QVector by value not only imposes the cost of copying it,
    but creates a risk of detaching if not used carefully: even when a
    Path is constant, `Path::segments() const` returns a temporary,
    non-const QVector. So the users of this function should take care to
    call only purely const QVector member functions or wrap segments()
    in as_const or assign segments() to a temporary constant variable.

Initially this comparison optimization was implemented in
ProjectFileDataProvider::projectClosing(). Milian suggested moving the
logic into Path::operator== to speed up Path comparison everywhere.

The current item removal algorithm in
ProjectFileDataProvider::projectClosing() works perfectly for a single
project or for several not very large projects. But when there is at
least one huge project (such as WebKit) and many small projects opened
at the same time, closing even a tiny project requires iterating over
all files and comparing each item's projectPath to the path of the
project that is being closed, which takes a long time if the project
directories are located in a common parent directory.

Comparing Path segments in reverse order is much faster in this case,
because project directory names are compared first. The comparison
practically always ends no later than at the last Path segments (now
checked first). That's because the project directory names of two
projects are almost never equal as KDevelop doesn't support opening two
projects with the same name (BUG 210562). And when the paths are equal,
Path::operator== returns right away, because in practice all
ProjectFile::projectPath objects share QVector data with constant
IProject::path().

For example, consider closing many projects at KDevelop exit, the last
two of which are WebKit with almost 320 thousand files and kdevelop with
a little more than 67 thousand files. Without this commit closing a
project (that may even contain less than a thousand files) located in
the same directory as WebKit and kdevelop, takes 47 ms on average if its
files precede the WebKit's files in m_projectFiles and 27 ms on average
otherwise. Closing a project that has a projectPath with a number of
segments unequal to that of WebKit and kdevelop takes just 1 ms, because
QVector::operator== returns early if the container sizes are different.
Closing the penultimate WebKit project takes 67 ms on average.

At this commit the 47 ms and 27 ms cited above turn into 3.6 ms. The
1 ms case is unaffected by this optimization. The 67 ms turn into 59 ms.

I have attempted an alternative optimization of projectClosing(), with
which closing any small project took just 1 ms - several times less than
at this commit. The idea was to store const IProject* project instead of
Path projectPath in ProjectFile struct. However, it is difficult to
ensure that the pointer is not dereferenced after the project is closed
and destroyed. Doing so requires either:
  * Increasing sizeof(ProjectFile) from 24 bytes to 32 bytes to store
    both the pointer and the path or to store a QPointer. Such a change
    is likely to negatively affect performance of other functions.
  * Synchronously calling QuickOpenDataProviderBase::reset() when a
    project is closed. This has negative performance consequences if a
    project is closed, then opened again before Quick Open line edit is
    activated; or if a project is reopened.

Yet another alternative optimization is to remove files from multiple
projects in one iteration over m_projectFiles when these projects are
closed in quick succession (this happens at KDevelop exit). But merging
successive invocations of projectClosing() and processing them once with
help of a timer, or introducing an IProjectController::projectsClosing()
signal, require many code changes and are difficult to get right.

This change speeds up ProjectFileDataProvider::projectClosing() during
KDevelop exit while a single project - kdevelop itself with a little
more than 67 thousand files - is open, from 14 ms to 13 ms on average.
In case of WebKit with its almost 320 thousand files as the single
project - from 56 ms to 54 ms.

When the last project is closed, there is a unique opportunity to free
the memory of m_projectFiles without an extra allocation and without
moving any ProjectFile objects. `m_projectFiles = {}` in place of
`m_projectFiles.clear()` does not affect the execution time of
projectClosing(). However releasing the memory can slow down the next
call to ProjectFileDataProvider::projectOpened(). When the capacity of
m_projectFiles is 0, projectOpened() has to repeatedly reallocate memory
and move ProjectFile objects while iterating over the files of the just
opened project. When all projects are closed, the user is likely exiting
KDevelop (in which case `= {}` vs `clear()` does not matter) or is going
to open another project soon.

Even when a huge project with 300 thousand files is closed, at most mere
15 MB could be released here, because m_projectFiles.capacity() would be
at most 600 thousand and sizeof(ProjectFile) == 24.

The current algorithm iterates over project files and erases matching
items from QVector m_projectFiles one by one. This is very slow, because
KDevelop::forEachFile() iterates over project files in an order close to
lexicographic, and m_projectFiles is sorted in lexicographic order too.
So the average position of an erased item is close to QVector::begin().
The overall complexity of this algorithm is O(N*M), where N is the size
of m_projectFiles before a project is closed, and M is the number of
files in the project that is being closed.

Simply iterating over the project files in the reverse order speeds up
the erase one-by-one algorithm a lot. But there is an even faster
alternative, which is implemented in this commit: instead of iterating
over or collecting project files and then removing matching items from
m_projectFiles, remove all items that have projectPath equal to the path
of the project that is being closed. The complexity of this algorithm is
O(N).

Before this commit on average 3150 ms were spent in
ProjectFileDataProvider::projectClosing() during KDevelop exit while a
single project - kdevelop itself with a little more than 67 thousand
files - was open. At this commit the time is just 14 ms. When the single
project to be closed was WebKit with its almost 320 thousand files,
about 196 *seconds* were spent in this function before the commit and
only 56 ms at this commit.

The best time I could achieve for closing kdevelop project by iterating
over project files in the reverse order and applying minor heuristic
optimizations was 165 ms. And such a reverse iteration could still be
extremely slow when several projects were closed.

This change makes
BenchQuickOpen::benchProjectFileFilter_addRemoveProject() 1.5 times
faster for a project with 100 files; 1.7 times faster for a project with
500 files; 2.2 times faster for a project with 1000 files; 5.9 times
faster for a project with 10000 files.

This commit changes behavior in case when a single file belongs to
multiple projects. Previously all files belonging to the project being
closed were removed. Now only the files that were added in the matching
projectOpened() call are removed. Neither the previous nor the current
behavior is ideal. But removing the files that were added along with the
project makes a bit more sense to me.

CCBUG: 427481

By doing more :) QTestLib otherwise detecs that the time required
for a single loop is too fast, it then retries with a higher iteration
count but that requires running the setup code again. And that is
actually way slower than the stuff we want to benchmark here...

So increase the workload of the benchmark code, or altneratively use
QBENCHMARK_ONCE with a large inner loop count.

Previously, the whole benchmark took ~25s on my system. Now it only
takes ~7s and gives better data too.

See previous commit, this test is also affected by it.

Apparently we end up in either the same line of the previous call
or the line of the next call when we step out. I guess this depends
on the version of GCC/Clang or GDB that's used. Allow both scenarios.

Don't try to parse a version via a regex, just QVersionNumber
instead. While at it, port to QRegularExpression and don't
crash if the response list is empty.

Don't try to parse a version via a regex, just QVersionNumber
instead. While at it, port to QRegularExpression and don't
crash if the response list is empty.

Allow multiple initializations of the AutoTestShell instead of
asserting.

Fixes LSAN report:
```

Indirect leak of 16 byte(s) in 1 object(s) allocated from:
    #0 0x7fcdeefc1f41 in operator new(unsigned long) /build/gcc/src/gcc/libsanitizer/asan/asan_new_delete.cpp:99
    #1 0x7fcdeec2ec62 in KDevelop::KDevSignalSpy::KDevSignalSpy(QObject*, char const*, Qt::ConnectionType) /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/kdevplatform/tests/kdevsignalspy.cpp:34
    #2 0x55f03429e6a2 in loadProject /home/milian/projects/kf5/src/extragear/kdevelop/kdevelop/plugins/custom-definesandincludes/tests/projectsgenerator.cpp:44
```

Actually dispose the translation units...

In case of conflict in i18n, keep the version of the branch "ours"
To resolve a particular conflict, "git checkout --ours path/to/file.desktop"

In case of conflict in i18n, keep the version of the branch "ours"
To resolve a particular conflict, "git checkout --ours path/to/file.desktop"