This is a multi-part message in MIME format. --nextPart1881018.7UT22OtMio Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Am Mittwoch, 4. Dezember 2013, 16:36:12 schrieb Milian Wolff: > On Wednesday 04 December 2013 16:01:03 Frank Reininghaus wrote: > > Hi, > > > > 2013/11/30 Kevin Funk: > > > Hey guys, > > > > > > My recent attempts to make KDevelop shine on Windows revealed some odd > > > behaviour inside kdelibs. > > > > > > I need some help in order to find the right approach to fix an at least > > > 3 > > > year old bug [1] ultimately caused by the use of a static member object > > > in the KCompTreeNode class (kcompletion.h) in kdelibs. Please see the > > > attached bug report [2]. > > > > > > Note that the error *only shows up on Windows*, on Linux we don't have > > > this > > > problem. I'll try to explain what's wrong in this mail. > > > > > > The actual issue: > > > - KDevelop running: > > > - We have an instance of KateGlobal owned by KateFactory, > > > > > > a plugin loaded by KDevelop > > > > > > - Some descendant of KateGlobal (KateCmd) owns a KCompTreeNode instance > > > - KCompTreeNode uses a custom allocator for creating/deleting instances > > > > > > of itself, holds a static member of type KZoneAllocactor for that > > > > > > - KZoneAllocator holds the memory for all the instantiated > > > KCompTreeNodes > > > > > > So we have the following object ownership (some levels omitted): > > > - KateFactory > > > > > > KateGlobal > > > > > > KCompTreeNode > > > > > > KZoneAllocator (static) > > > > > > When closing KDevelop *under Linux*: > > > - ~KateFactory, via QObjectCleanupHandler during __run_exit_handlers > > > (glibc)> > > > > > > ~KateGlobal, via call to KateGlobal::decRef() > > > > > > ~KCompTreeNode > > > > > > - ~KZoneAllocator, during __cxa__finalize (glibc) > > > > > > The order is fine in this case. No segfaults. > > > > > > However, on Windows the destruction order is different (and I cannot > > > really > > > explain why). When closing KDevelop *under Windows*: > > > - ~KZoneAllocator is called first, via > > > > > > "kdeui.dll!`dynamic atexit destructor for > > > 'KCompTreeNode::alloc''()" > > > > > > - ~KateFactory, via QObjectCleanupHandler > > > > > > ~KateGlobal > > > > > > Call to KCompTreeNode::removeItem() > > > > > > => Crash because memory for the KCompTreeNode instances was already > > > freed > > > > > > by ~KZoneAllocator > > > > > > I wonder how to fix that: > > > The easiest solution obviously: Get rid off the custom allocator in > > > KCompTreeNode. Having a class owning its custom allocator is just plain > > > wrong, right? But removing that probably has performance impacts. (Does > > > anyone have benchmarks here?) > > > > well, having a custom allocator may have some benefits. I guess that > > KCompletion uses one to ensure that all KCompTreeNodes are close to > > each other in memory and thus to reduce cache misses? I don't know how > > much dropping the custom allocator would affect the performance, one > > would have to do some benchmarking with typical KCompletion use cases. > > I agree that it might be a good performance improvement. But I want to note > neither KDevelop nore Kate uses this technique for its code completion in > the editor. And we probably have much more completion tree nodes than a > usual KCompletion user. I just mention this to show that removing this > performance improvement should not kill the performance, rather decrease it > slightly. > > > Just using K_GLOBAL_STATIC for the KZoneAllocator instance doesn't help > > > either. I get the same destruction order + crash with that on Windows. > > > > > > Dynamic allocation inside KCompTreeNode doesn't work either, because > > > KZoneAllocator needs to be there *before* the first KCompTreeNode is > > > created and needs to there *until* the last one has been destroyed. > > > > > > Any ideas? > > > > Well, here is one idea (untested): use a > > QSharedPointer to ensure that the KZoneAllocator is > > not destroyed before the last KCompletion instance using it. > > > > Replace > > > > static KZoneAllocator alloc; > > > > in KCompTreeNode by > > > > static QSharedPointer alloc; > > > > and, in kcompletion.cpp, > > > > KZoneAllocator KCompTreeNode::alloc(8192); > > > > by > > > > QSharedPointer KCompTreeNode::alloc; > > > > In KCompTreeNode's operator new/delete, replace "alloc." by "alloc->". > > > > Furthermore, add a QSharedPointer member to > > KCompletionPrivate. In KCompletionPrivate's constructor, first check > > if KCompTreeNode::alloc isNull(), initialize it if that is not the > > case, and then copy "alloc" to the new member. > > > > I think that this might work, and it might also improve application > > start-up time because the KZoneAllocator would only be initialized on > > first use of a KCompletion object. Hey Frank, thanks a lot for sharing your thoughts! I just tried your suggestion, and it indeed fixes the issue on Windows (and doesn't cause havoc on Linux either). See attached patch (v1). I think it pretty much resembles your thoughts. However, I'm not quite sure this approach is going to be accepted upstream. One issue, for example, is that KCompTreeNode now has a publicly accessible static KZoneAllocator* member and explicitly setting the allocator from within KCompletion seems odd. That completely makes KCompTreeNode(s) depend on KCompletion. I'm proposing another approach: - Still using QSharedPointer - But: Initialize this shared-pointer directly (as with the static KZA member) - And: Keep a strong-ref in KCompletion on that shared pointer See attached patch (v2) Any objections? Otherwise I'll post that patch to reviewboard ASAP. > (snip) Greets -- Kevin Funk --nextPart1881018.7UT22OtMio Content-Disposition: attachment; filename="0001-Attempt-to-fix-KZoneAllocator-issue_v2.patch" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="utf-8"; name="0001-Attempt-to-fix-KZoneAllocator-issue_v2.patch" From e6717bdd347b884631c1e9f44f1476f63c87fce9 Mon Sep 17 00:00:00 2001 From: Kevin Funk Date: Wed, 11 Dec 2013 10:48:00 +0100 Subject: [PATCH] Attempt to fix KZoneAllocator issue --- kdecore/util/kallocator.cpp | 8 +++++--- kdeui/util/kcompletion.cpp | 5 ++++- kdeui/util/kcompletion_p.h | 14 +++++++++++--- 3 files changed, 20 insertions(+), 7 deletions(-) diff --git a/kdecore/util/kallocator.cpp b/kdecore/util/kallocator.cpp index 8b21120..5e3f4eb 100644 --- a/kdecore/util/kallocator.cpp +++ b/kdecore/util/kallocator.cpp @@ -28,9 +28,11 @@ */ #include "kallocator.h" -#include "kdebug.h" + #include +#include + class KZoneAllocator::MemBlock { public: @@ -105,9 +107,9 @@ KZoneAllocator::~KZoneAllocator() count++; } #ifndef NDEBUG // as this is called quite late in the app, we don't care - // to use kDebug + // to use qDebug if (count > 1) - qDebug("zone still contained %d blocks", count); + fprintf(stderr, "zone still contained %d blocks", count); #endif delete d; } diff --git a/kdeui/util/kcompletion.cpp b/kdeui/util/kcompletion.cpp index 340aa92..01c4273 100644 --- a/kdeui/util/kcompletion.cpp +++ b/kdeui/util/kcompletion.cpp @@ -33,6 +33,7 @@ class KCompletionPrivate public: KCompletionPrivate() : myCompletionMode( KGlobalSettings::completionMode() ) + , mTreeNodeAllocator( KCompTreeNode::allocator() ) // keep strong-ref to allocator instance , myTreeRoot( new KCompTreeNode ) , myBeep( true ) , myIgnoreCase( false ) @@ -49,6 +50,8 @@ public: KGlobalSettings::Completion myCompletionMode; + QSharedPointer mTreeNodeAllocator; + KCompletion::CompOrder myOrder; QString myLastString; QString myLastMatch; @@ -983,6 +986,6 @@ KCompTreeNode *KCompTreeNodeList::at(uint index) const return cur; } -KZoneAllocator KCompTreeNode::alloc(8192); +QSharedPointer KCompTreeNode::alloc(new KZoneAllocator(8*1024)); #include "kcompletion.moc" diff --git a/kdeui/util/kcompletion_p.h b/kdeui/util/kcompletion_p.h index 1cf31db..d31b7db 100644 --- a/kdeui/util/kcompletion_p.h +++ b/kdeui/util/kcompletion_p.h @@ -94,10 +94,12 @@ public: ~KCompTreeNode(); void * operator new( size_t s ) { - return alloc.allocate( s ); + Q_ASSERT(alloc); + return alloc->allocate( s ); } void operator delete( void * s ) { - alloc.deallocate( s ); + Q_ASSERT(alloc); + alloc->deallocate( s ); } // Returns a child of this node matching ch, if available. @@ -135,10 +137,16 @@ public: need to use QValueList<>. And to make it even more fast we don't use an accessor, but just a public member. */ KCompTreeNode *next; + + /** + * Custom allocator used for all KCompTreeNode instances + */ + static QSharedPointer allocator() { return alloc; } + private: uint myWeight; KCompTreeNodeList myChildren; - static KZoneAllocator alloc; + static QSharedPointer alloc; }; -- 1.8.3.2 --nextPart1881018.7UT22OtMio Content-Disposition: attachment; filename="0001-Attempt-to-fix-KZoneAllocator-issue_v1.patch" Content-Transfer-Encoding: 7Bit Content-Type: text/x-patch; charset="utf-8"; name="0001-Attempt-to-fix-KZoneAllocator-issue_v1.patch" From 5ae70e2ece5e34a5750a52fb05b7e3b7e95ba713 Mon Sep 17 00:00:00 2001 From: Kevin Funk Date: Wed, 11 Dec 2013 10:48:00 +0100 Subject: [PATCH] Attempt to fix KZoneAllocator issue --- kdecore/util/kallocator.cpp | 8 +++++--- kdeui/util/kcompletion.cpp | 11 +++++++++-- kdeui/util/kcompletion_p.h | 14 +++++++++++--- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/kdecore/util/kallocator.cpp b/kdecore/util/kallocator.cpp index 8b21120..5e3f4eb 100644 --- a/kdecore/util/kallocator.cpp +++ b/kdecore/util/kallocator.cpp @@ -28,9 +28,11 @@ */ #include "kallocator.h" -#include "kdebug.h" + #include +#include + class KZoneAllocator::MemBlock { public: @@ -105,9 +107,9 @@ KZoneAllocator::~KZoneAllocator() count++; } #ifndef NDEBUG // as this is called quite late in the app, we don't care - // to use kDebug + // to use qDebug if (count > 1) - qDebug("zone still contained %d blocks", count); + fprintf(stderr, "zone still contained %d blocks", count); #endif delete d; } diff --git a/kdeui/util/kcompletion.cpp b/kdeui/util/kcompletion.cpp index 340aa92..351fb4b 100644 --- a/kdeui/util/kcompletion.cpp +++ b/kdeui/util/kcompletion.cpp @@ -33,12 +33,17 @@ class KCompletionPrivate public: KCompletionPrivate() : myCompletionMode( KGlobalSettings::completionMode() ) - , myTreeRoot( new KCompTreeNode ) + , myTreeRoot( 0 ) , myBeep( true ) , myIgnoreCase( false ) , myHasMultipleMatches( false ) , myRotationIndex( 0 ) { + if (KCompTreeNode::alloc.isNull()) { + KCompTreeNode::alloc = QSharedPointer(new KZoneAllocator); + treeNodeAllocator = KCompTreeNode::alloc; // keep strong-ref to allocator instance + } + myTreeRoot = new KCompTreeNode; } ~KCompletionPrivate() { @@ -49,6 +54,8 @@ public: KGlobalSettings::Completion myCompletionMode; + QSharedPointer treeNodeAllocator; + KCompletion::CompOrder myOrder; QString myLastString; QString myLastMatch; @@ -983,6 +990,6 @@ KCompTreeNode *KCompTreeNodeList::at(uint index) const return cur; } -KZoneAllocator KCompTreeNode::alloc(8192); +QSharedPointer KCompTreeNode::alloc; #include "kcompletion.moc" diff --git a/kdeui/util/kcompletion_p.h b/kdeui/util/kcompletion_p.h index 1cf31db..d21d627 100644 --- a/kdeui/util/kcompletion_p.h +++ b/kdeui/util/kcompletion_p.h @@ -94,10 +94,12 @@ public: ~KCompTreeNode(); void * operator new( size_t s ) { - return alloc.allocate( s ); + Q_ASSERT(alloc); + return alloc->allocate( s ); } void operator delete( void * s ) { - alloc.deallocate( s ); + Q_ASSERT(alloc); + alloc->deallocate( s ); } // Returns a child of this node matching ch, if available. @@ -135,10 +137,16 @@ public: need to use QValueList<>. And to make it even more fast we don't use an accessor, but just a public member. */ KCompTreeNode *next; + + /** + * Custom allocator used for all KCompTreeNode instances + * @warning Assign instance of KZoneAllocator before using KCompTreeNodes + */ + static QSharedPointer alloc; + private: uint myWeight; KCompTreeNodeList myChildren; - static KZoneAllocator alloc; }; -- 1.8.3.2 --nextPart1881018.7UT22OtMio Content-Type: text/plain; charset="us-ascii" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit Content-Disposition: inline >> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe << --nextPart1881018.7UT22OtMio--