Re: Review Request 114715: Attempt to fix KZoneAllocator issue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review49478 --- This review has been submitted with commit c34045e2a2263865c825927de44c51faf5926132 by Albert Astals Cid on behalf of Kevin Funk to branch KDE/4.12. - Commit Hook On Jan. 28, 2014, 1:22 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 28, 2014, 1:22 a.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
On Jan. 24, 2014, 4:32 p.m., David Faure wrote: Urgh - a static object in a library? How did we not notice before... Needs to be fixed indeed. I would have used Q_GLOBAL_STATIC (which has methods like isDestroyed()), but I guess that's equivalent to your sharedpointer + strong ref idea, so why not. Kevin Funk wrote: Where do you want me to commit this? Both to kdelibs master + kcompletion framework? Yes. - David --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review48232 --- On Jan. 3, 2014, 5:52 p.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 3, 2014, 5:52 p.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review48434 --- This review has been submitted with commit 8beb63bcb08568eac8578844d7a1f4f44280f685 by Kevin Funk to branch master. - Commit Hook On Jan. 3, 2014, 5:52 p.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 3, 2014, 5:52 p.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review48435 --- This review has been submitted with commit d10d52e0cc6b57f01dbe92a11eea94d0b12aa166 by Kevin Funk to branch master. - Commit Hook On Jan. 28, 2014, 1:22 a.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 28, 2014, 1:22 a.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
On Jan. 24, 2014, 4:32 p.m., David Faure wrote: Urgh - a static object in a library? How did we not notice before... Needs to be fixed indeed. I would have used Q_GLOBAL_STATIC (which has methods like isDestroyed()), but I guess that's equivalent to your sharedpointer + strong ref idea, so why not. Where do you want me to commit this? Both to kdelibs master + kcompletion framework? - Kevin --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review48232 --- On Jan. 3, 2014, 5:52 p.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 3, 2014, 5:52 p.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review48232 --- Ship it! Urgh - a static object in a library? How did we not notice before... Needs to be fixed indeed. I would have used Q_GLOBAL_STATIC (which has methods like isDestroyed()), but I guess that's equivalent to your sharedpointer + strong ref idea, so why not. - David Faure On Jan. 3, 2014, 5:52 p.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 3, 2014, 5:52 p.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/#review47255 --- As discussed on the mailing list, I think that this is a valid approach to ensure that the custom allocator is not deleted before the last KCompletion instance using it. The change might not only be beneficial on Windows - maybe the code is a time bomb waiting to explode also on other platforms. I think it would be slightly nicer to initialize the allocator when it's first used, rather than when the library is loaded, but this is unrelated to the purpose of this patch. Maybe something to consider for Frameworks, to which this patch must be ported manually anyway? (Another question for Frameworks would obviously be how much the performance is actually improved by the custom allocator, and thus, if it's worth keeping it. As I said in http://lists.kde.org/?l=kde-develm=138684405315855w=1, there were some claims in 2002 that it greatly speeds up KCompletion, but I'm not sure if these claims can (still) be trusted). In any case, I don't have good knowledge of KCompletion, so I don't feel qualified to say Ship It! (at least not before waiting another week or two for possible comments from others). - Frank Reininghaus On Jan. 3, 2014, 5:52 p.m., Kevin Funk wrote: --- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 3, 2014, 5:52 p.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
Re: Review Request 114715: Attempt to fix KZoneAllocator issue
--- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/114715/ --- (Updated Jan. 3, 2014, 5:52 p.m.) Review request for kdelibs and Frank Reininghaus. Bugs: 327599 http://bugs.kde.org/show_bug.cgi?id=327599 Repository: kdelibs Description --- Attempt to fix KZoneAllocator issue kcompletion.p_h: Make the static KZoneAllocator member of KCompTreeNode a shared pointer so external users can hold a reference to it. kcompletion.cpp: Hold a reference to KCompTreeNode's KZoneAllocator instance so we avoid deleting the KZoneAllocator too early. See attached bug report for possible causes. (Hint: It crashes on Windows because ~KZoneAllocator is called to early.) kallocator.cpp: Use printf instead of qDebug(), because this code path code might be called very late during destruction and qDebug() will crash deep inside Qt. Also see discussion: http://lists.kde.org/?l=kde-develm=138583383708455w=1 BUG: 327599 CCBUG: 243375 Diffs - kdecore/util/kallocator.cpp 8b21120c62c513ea41686fe8185ec2808fe5d83a kdeui/util/kcompletion.cpp 340aa92b900d670e2ad73f70a63d5221d0feed1d kdeui/util/kcompletion_p.h 1cf31db3f16fe3421415cd54265eee20bb998710 Diff: https://git.reviewboard.kde.org/r/114715/diff/ Testing --- Thanks, Kevin Funk
