Review Request 118587: Optimize KConfigIniBackend::parseConfig by reducing allocations.

[Milian Wolff]

> On June 6, 2014, 10:59 a.m., David Faure wrote:
> > kdecore/config/bufferfragment_p.h, line 36
> > <https://git.reviewboard.kde.org/r/118587/diff/1/?file=279294#file279294line36>
> >
> >     Why? Now it could clash with an application class BufferFragment, on a system without "hidden visibility". Or it might confuse gdb, even with hidden-visibility? not sure how that works.

Because I need to access it from free functions (qHash, lookup). I can also make it a public if you prefer to keep the namespace. Is that OK?


> On June 6, 2014, 10:59 a.m., David Faure wrote:
> > kdecore/config/kconfigini.cpp, line 109
> > <https://git.reviewboard.kde.org/r/118587/diff/1/?file=279295#file279295line109>
> >
> >     This isn't always the case though. kmail does that, yes, but e.g. kdeglobals doesn't.
> >     Nor most desktop files, nor konquerorrc (except that some video player added per-file groups into mine!).
> >     
> >     What do we lose if the config file has no repeated keys? Memory (the hash) and CPU (inserting and looking up into the hash), right?
> >     
> >     I have a hard time being sure that the tradeoff is always in our favour.

Yes, the tradeoff is the memory of the hash-internal structure (which is temporary!).

Looking at my konquerorrc, I still see tons of shared strings: false and true :) Then all the stuff in the toolbar configuration, esp. the keys used there.

Also: konquerorrc and other config or desktop files are small. It might be slowed down by this patch, but not by much. It will not become a hotspot suddenly. But the cases where this already is an issue right now, this patch helps a lot.


- Milian


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


On June 6, 2014, 9:31 a.m., Milian Wolff wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118587/
> -----------------------------------------------------------
> 
> (Updated June 6, 2014, 9:31 a.m.)
> 
> 
> Review request for kdelibs and David Faure.
> 
> 
> Repository: kdelibs
> 
> 
> Description
> -------
> 
> Optimize KConfigIniBackend::parseConfig by reducing allocations.
> 
> Yet another awesome application of the Qt implicit sharing trick.
> Since config files often contain only few different keys and even
> value strings, we can share them. This reduces memory consumption
> and also speeds up parsing, as we do not have to allocate the
> duplicated strings, but can simply reuse the previous values.
> 
> The most extreme case for this of my knowledge, is KatePart:
> katesyntaxhighlightingrc has more then 20k lines which triggered
> about 30k allocations on startup. With this patch applied, this
> value goes down dramatically. I added a simple static counter for
> the cache hit/miss ratio, which resulted in 5442 cache misses compared
> to 43624 cache hits across all KConfig files parsed by kwrite on startup.
> 
> 
> Diffs
> -----
> 
>   kdecore/config/bufferfragment_p.h 5a753ad 
>   kdecore/config/kconfigini.cpp 8ec43c5 
>   kdecore/config/kconfigini_p.h d7aa43e 
> 
> Diff: https://git.reviewboard.kde.org/r/118587/diff/
> 
> 
> Testing
> -------
> 
> Unit tests all pass. My malloc tracer shows that the allocations are all gone.
> 
> My malloc tracer showed before:
> 
> 17421 allocations at:
> 0x7fee73692b97 QByteArray::QByteArray(char const*, int) /usr/lib/libQtCore.so.4
> 0x7fee73bb7cee ? /usr/lib/libkdecore.so.5
> 0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags<KConfig::OpenFlag>, char const*) /usr/lib/libkdecore.so.5
> 0x7fee64830c06 KateHlManager::KateHlManager() in /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
> 
> 12699 allocations at:
> 0x7fee73692b97 QByteArray::QByteArray(char const*, int) /usr/lib/libQtCore.so.4
> 0x7fee73bb7cd7 ? /usr/lib/libkdecore.so.5
> 0x7fee73bb7fc4 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1320 ? /usr/lib/libkdecore.so.5
> 0x7fee73ba1731 KConfig::KConfig(QString const&, QFlags<KConfig::OpenFlag>, char const*) /usr/lib/libkdecore.so.5
> 0x7fee64830c06 KateHlManager::KateHlManager() in /ssd/milian/projects/kde4/kate/part/syntax/katesyntaxmanager.cpp:76 /ssd/milian/projects/compiled/kde4/lib/libkatepartinterfaces.so.4
> 
> These where the allocation hotspots number #1 and #3 respectively. With the patch applied, the two locations are not under the top 10 anymore.
> 
> 
> Thanks,
> 
> Milian Wolff
> 
>

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