[prev in list] [next in list] [prev in thread] [next in thread]
List: kde-devel
Subject: Re: Problem with KCompTreeNode on Windows (or: destruction order of static objects?)
From: Kevin Funk <krf () gmx ! de>
Date: 2013-12-11 22:52:51
Message-ID: 6509864.voR4U5qZp3 () kerberos
[Download RAW message or body]
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<KZoneAllocator> to ensure that the KZoneAllocator is
> > not destroyed before the last KCompletion instance using it.
> >
> > Replace
> >
> > static KZoneAllocator alloc;
> >
> > in KCompTreeNode by
> >
> > static QSharedPointer<KZoneAllocator> alloc;
> >
> > and, in kcompletion.cpp,
> >
> > KZoneAllocator KCompTreeNode::alloc(8192);
> >
> > by
> >
> > QSharedPointer<KZoneAllocator> KCompTreeNode::alloc;
> >
> > In KCompTreeNode's operator new/delete, replace "alloc." by "alloc->".
> >
> > Furthermore, add a QSharedPointer<KZoneAllocator> 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<KZoneAllocator>
- 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
["0001-Attempt-to-fix-KZoneAllocator-issue_v2.patch" (0001-Attempt-to-fix-KZoneAllocator-issue_v2.patch)]
From e6717bdd347b884631c1e9f44f1476f63c87fce9 Mon Sep 17 00:00:00 2001
From: Kevin Funk <kevin@kfunk.org>
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 <QList>
+#include <stdio.h>
+
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<KZoneAllocator> 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<KZoneAllocator> 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<KZoneAllocator> allocator() { return alloc; }
+
private:
uint myWeight;
KCompTreeNodeList myChildren;
- static KZoneAllocator alloc;
+ static QSharedPointer<KZoneAllocator> alloc;
};
--
1.8.3.2
["0001-Attempt-to-fix-KZoneAllocator-issue_v1.patch" (0001-Attempt-to-fix-KZoneAllocator-issue_v1.patch)]
From 5ae70e2ece5e34a5750a52fb05b7e3b7e95ba713 Mon Sep 17 00:00:00 2001
From: Kevin Funk <kevin@kfunk.org>
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 <QList>
+#include <stdio.h>
+
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<KZoneAllocator>(new KZoneAllocator);
+ treeNodeAllocator = KCompTreeNode::alloc; // keep strong-ref to allocator instance
+ }
+ myTreeRoot = new KCompTreeNode;
}
~KCompletionPrivate()
{
@@ -49,6 +54,8 @@ public:
KGlobalSettings::Completion myCompletionMode;
+ QSharedPointer<KZoneAllocator> 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<KZoneAllocator> 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<KZoneAllocator> alloc;
+
private:
uint myWeight;
KCompTreeNodeList myChildren;
- static KZoneAllocator alloc;
};
--
1.8.3.2
>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]
Configure |
About |
News |
Add a list |
Sponsored by KoreLogic