[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:       Frank Reininghaus <frank78ac () googlemail ! com>
Date:       2013-12-12 10:27:07
Message-ID: CAFoZWWhr730GtTgaikLs==hnnXVQ1t-HJwdux+zo9PTc_HeAhg () mail ! gmail ! com
[Download RAW message or body]

Hi,

2013/12/11 Kevin Funk:
[...]
>> > > 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.

great, I'm glad to hear that! I'd recommend to remove the unrelated
changes in KZoneAllocator though - they just make the patches harder
to understand.

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

well, I guess it might be considered a matter of taste. The reason why
I find v1 slightly better is that the initialization of the custom
allocator is deferred until it is really needed. Right now, the
KZoneAllocator constructor will be called on startup for every
application that links to kdeui.

BTW, I found the discussion about the introduction of KZoneAllocator
in KCompletion:

http://lists.kde.org/?t=101548827800001&r=1&w=2

The author claims that

"A changed kcompletiontest which just add a big number of strings in
different ways, and clears the completion object in between showed more
than a double speedup (from 4950 to 2100 msec)"

However, it seems that this "changed" test is not provided, and
moreover, the patch changes a lot more than just the allocator, so
there is no way to know which part of the saving is due to which
change. IMHO, two good reasons why this patch should not have been
committed without further discussions, but I guess that my objection
is more than 11 years too late ;-)

(BTW, if I wrote a custom allocator for a class which always allocates
objects of the same size, I would try to make use of that in order to
get rid of all the overhead that is needed to keep track of the size
of the allocated blocks, but that's not the topic of this thread.)

Cheers,
Frank

>> Visit http://mail.kde.org/mailman/listinfo/kde-devel#unsub to unsubscribe <<
[prev in list] [next in list] [prev in thread] [next in thread]