Review Request 118586: Optimize KConfigGroup::exists and similar operations.

[Milian Wolff]

> On June 6, 2014, 8:48 a.m., David Faure wrote:
> > kdecore/config/kconfig.cpp, line 337
> > <https://git.reviewboard.kde.org/r/118586/diff/1/?file=279281#file279281line337>
> >
> >     isn't this removal going to affect all callers of allSubGroups, possibly creating bugs?
> >     
> >     hasNonDeletedEntries doesn't call allSubGroups anymore, but deleteGroupImpl does. Are you sure that it's unaffected by this change?

Well, sure - it could fail. But that's what the unit tests are there for, no?

The isGroupOrSubGroupMatch also takes the parentGroup into account. So I don't have to add it upfront, it will be added later on. If I mess with that function to exclude the parent group, the unit tests start to fail:

FAIL!  : KConfigTest::testDelete() '!delgr.exists()' returned FALSE. ()
   Loc: [/ssd/milian/projects/kde4/kdelibs/kdecore/tests/kconfigtest.cpp(702)]
FAIL!  : KConfigTest::testDeleteWhenLocalized() '!cg.hasKey("foo")' returned FALSE. ()
   Loc: [/ssd/milian/projects/kde4/kdelibs/kdecore/tests/kconfigtest.cpp(1497)]
FAIL!  : KConfigTest::testDefaultGroup() 'lines.first() != QByteArray("TestKey=defaultGroup\n")' returned FALSE. ()
   Loc: [/ssd/milian/projects/kde4/kdelibs/kdecore/tests/kconfigtest.cpp(797)]
FAIL!  : KConfigTest::testEmptyGroup() '!emptyGroup.exists()' returned FALSE. ()
   Loc: [/ssd/milian/projects/kde4/kdelibs/kdecore/tests/kconfigtest.cpp(805)]

so again, I'm pretty confident that this patch is fine.


- Milian


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118586/#review59386
-----------------------------------------------------------


On June 6, 2014, 8:17 a.m., Milian Wolff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118586/
> -----------------------------------------------------------
> 
> (Updated June 6, 2014, 8:17 a.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> Optimize KConfigGroup::exists and similar operations.
> 
> Before, these kind of read-only operations did a lot of allocations:
> 
> 1) allocate a list of all sub groups
> 2) for the above, also allocate a sub-group match key
> 3) iterate over sub groups, allocate a list of all keys in there
> and then finally check whether that list is non-empty
> 
> All of the above is now done without a single allocation, by simply
> iterating over the list of entries.
> 
> Note: The whole list was iterated even before in allSubGroups. Now
> we still do that, but check for non-empty keys in the group or
> sub group directly. Much more efficient.
> 
> Note2: While at it, a duplicate comparison in keyListImpl is removed.
> Furthermore, allSubGroups is also optimized to not require the
> allocation of the subgroup match key.
> 
> 
> Diffs
> -----
> 
>   kdecore/config/kconfig.cpp 790227ad7531acb8daa00f739267737247d9b7a0 
> 
> Diff: https://git.reviewboard.kde.org/r/118586/diff/
> 
> 
> Testing
> -------
> 
> The unit tests all run just fine. I broke it while implementing this patch, so I'm confident this functionality is actually properly tested and covered :)
> 
> The saved allocations are plenty. This is e.g. from the startup of kwrite:
> 
> 2844 allocations at:
> 0x7ff7fbc490e9 QHashData::allocateNode(int) /usr/lib/libQtCore.so.4
> 0x7ff7fc152f8e QHash<QString, QHashDummyValue>::createNode(unsigned int, QString const&, QHashDummyValue const&, QHashNode<QString, QHashDumm
> yValue>**) in /usr/include/qt4/QtCore/qhash.h:543 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc152472 QHash<QString, QHashDummyValue>::insert(QString const&, QHashDummyValue const&) in /usr/include/qt4/QtCore/qhash.h:763 /ssd/mi
> lian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc1515f9 QSet<QString>::insert(QString const&) in /usr/include/qt4/QtCore/qset.h:181 /ssd/milian/projects/compiled/kde4/lib/libkdecore.
> so.5
> 0x7ff7fc1501ab QSet<QString>::operator<<(QString const&) in /usr/include/qt4/QtCore/qset.h:201 /ssd/milian/projects/compiled/kde4/lib/libkdec
> ore.so.5
> 0x7ff7fc14b4a7 KConfigPrivate::keyListImpl(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:367 /ssd/
> milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14b25d KConfigPrivate::hasNonDeletedEntries(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:
> 347 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14dfeb KConfig::hasGroupImpl(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:856 /ssd/milian
> /projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc1536b4 KConfigBase::hasGroup(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfigbase.cpp:42 /ssd/mil
> ian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc158fcd KConfigGroup::exists() const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfiggroup.cpp:592 /ssd/milian/projects/com
> piled/kde4/lib/libkdecore.so.5
> 
> 2844 allocations at:
> 0x7ff7fbc71426 QString::QString(int, Qt::Initialization) /usr/lib/libQtCore.so.4
> 0x7ff7fbd66820 ? /usr/lib/libQtCore.so.4
> 0x7ff7fbc737db QString::fromUtf8(char const*, int) /usr/lib/libQtCore.so.4
> 0x7ff7fc14b494 KConfigPrivate::keyListImpl(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:367 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14b25d KConfigPrivate::hasNonDeletedEntries(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:347 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc14dfeb KConfig::hasGroupImpl(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfig.cpp:856 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc1536b4 KConfigBase::hasGroup(QByteArray const&) const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfigbase.cpp:42 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 0x7ff7fc158fcd KConfigGroup::exists() const in /ssd/milian/projects/kde4/kdelibs/kdecore/config/kconfiggroup.cpp:592 /ssd/milian/projects/compiled/kde4/lib/libkdecore.so.5
> 
> All of these are gone with the patch.
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-core-devel/attachments/20140606/1aa4bae9/attachment.htm>