1. 14 Nov, 2020 4 commits
    • Igor Kushnir's avatar
      Make all DUChainReferenceCounting globals thread_local · 3c7257e5
      Igor Kushnir authored
      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.
      3c7257e5
    • Igor Kushnir's avatar
      Make the global variable doReferenceCounting thread_local · db67d592
      Igor Kushnir authored
      This variable is accessed concurrently from multiple threads. It is
      written to under a mutex lock, but read from without any thread
      synchronization. Therefore the variable has to be either atomic or
      thread_local to prevent data races.
      
      As far as I can tell, the variable's value is never intended to be
      shared with other threads. If one thread temporarily sets
      doReferenceCounting to true, and then another thread reads the variable
      before the first thread resets it to false, there is redundant mutex
      locking. If this other thread then (redundantly) sets
      doReferenceCounting to true, performs some operation and resets it to
      false; then the first thread reads this variable again, some necessary
      checks and steps can be skipped in the first thread. So thread_local
      seems to be the more correct and performant alternative.
      
      This change does not break any kdevelop or kdev-python tests.
      db67d592
    • Igor Kushnir's avatar
      Use std::as_const in place of custom make_const · 46158d1a
      Igor Kushnir authored
      GIT_SILENT
      46158d1a
    • Bernd Buschinski's avatar
      Fix excessive KPassivePopup usage in case of debugger errors · 8fca91a0
      Bernd Buschinski authored
      - Group duplicate error messages
      - Port to KNotification
      - This fixes a kwin_x11 crash for some X11 drivers
      8fca91a0
  2. 11 Nov, 2020 1 commit
  3. 10 Nov, 2020 1 commit
  4. 09 Nov, 2020 1 commit
  5. 07 Nov, 2020 1 commit
  6. 05 Nov, 2020 1 commit
  7. 04 Nov, 2020 4 commits
  8. 01 Nov, 2020 2 commits
  9. 30 Oct, 2020 12 commits
    • Milian Wolff's avatar
      Simplify manpagemodel and prevent leaks in test · 353a86e3
      Milian Wolff authored
      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.
      353a86e3
    • Milian Wolff's avatar
      Actually ensure that the test_qthelpplugin sees some data · 6ff9410a
      Milian Wolff authored
      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...
      6ff9410a
    • Milian Wolff's avatar
      Don't leak QProcess in QtHelp plugin · ba180bb3
      Milian Wolff authored
      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...
      ba180bb3
    • Milian Wolff's avatar
      Port private slot to lamda · 24e0750a
      Milian Wolff authored
      24e0750a
    • Milian Wolff's avatar
      Fix indentation and add non-inline dtor · fa62c04c
      Milian Wolff authored
      fa62c04c
    • Milian Wolff's avatar
      Don't leak deleted breakpoints · 5eeedbe5
      Milian Wolff authored
      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...
      5eeedbe5
    • Milian Wolff's avatar
      Explicitly disable sanitizers for debuggees · 927b758b
      Milian Wolff authored
      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.
      927b758b
    • Milian Wolff's avatar
      Don't leak QProcess in DebugSession::execInferior (remote debugging) · d9782419
      Milian Wolff authored
      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
      ```
      d9782419
    • Milian Wolff's avatar
    • Milian Wolff's avatar
      Fix memory leaks in bench_hashes · d4fbdbfe
      Milian Wolff authored
      d4fbdbfe
    • Milian Wolff's avatar
      DUChain: call delete on correct type · fa04bf7f
      Milian Wolff authored
      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
      ```
      fa04bf7f
    • Script Kiddy's avatar
      GIT_SILENT made messages (after extraction) · 1983ec2b
      Script Kiddy authored
      1983ec2b
  10. 29 Oct, 2020 1 commit
  11. 28 Oct, 2020 4 commits
    • Igor Kushnir's avatar
      Path::segments(): return a const reference instead of a copy · d8a7b7fc
      Igor Kushnir authored
      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.
      d8a7b7fc
    • Igor Kushnir's avatar
      Path::operator==: compare segments in reverse order · b13d377c
      Igor Kushnir authored
      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.
      b13d377c
    • Igor Kushnir's avatar
      ProjectFileDataProvider: clear items when no open projects left · 7477a4a1
      Igor Kushnir authored
      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.
      7477a4a1
    • Igor Kushnir's avatar
      ProjectFileDataProvider::projectClosing: match projectPath · 8e0f203a
      Igor Kushnir authored
      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
      8e0f203a
  12. 27 Oct, 2020 2 commits
    • Milian Wolff's avatar
      Reduce time required to run bench_quickopen · ae1aa195
      Milian Wolff authored
      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.
      ae1aa195
    • Script Kiddy's avatar
      GIT_SILENT made messages (after extraction) · aea68549
      Script Kiddy authored
      aea68549
  13. 26 Oct, 2020 6 commits