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