<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="https://git.reviewboard.kde.org/r/116461/">https://git.reviewboard.kde.org/r/116461/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On February 28th, 2014, 8:41 p.m. UTC, <b>Matthew Dawson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">While I'm fine with the idea behind this optimization, I worry that this implementation could create situations were a configuration change is not picked up by the system. For instance, what happens if the user doesn't immediately call readConfig? This could create some subtle bugs in downstream code.
I had two ideas for how this optimization could be implemented:
1) Lazy load the KConfig object the first time it is used. Then, in readConfig, the call to be reparseConfiguration could be avoided if the KConfig is created due to its call. This would retain the current behaviour, while ensuring readConfig reads in the most configuration. Other uses of the KConfig will have to ensure the KConfig object has already been created, and if the user calls one of those functions before readConfig, they will still double read the configuration. But since this is just status quo, I'm not too worried.
2) Alternately, create a set of construction functions, like make_shared, that imitate the creation of a KConfigSkeleton subclass, and then reading the configuration through readConfig. Internally, it can use a private readConfig function to ensure the configuration is no re-read. This would require changes to applications to avoid the extra configuration call, unfortunately.
I saw RR #115964, and I assume that some of the reductions to the readings of oxygenrc are caused by the sharing the KConfig between some KConfigSkeleton's? If so, I'd suggest implementing solution 1 for the general case, and then making a special construction function to handle shared KConfig's. I don't want to avoid calling reparseConfiguration without some warning around this, as it may again cause some surprises. A new appropriately named function shouldn't be too bad though, as opposed to changing the behaviour of the constructor.</pre>
</blockquote>
<p>On March 1st, 2014, 9:15 a.m. UTC, <b>David Faure</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I've been thinking about this too, but what good is a KConfigSkeleton if you don't call readSettings() on it? You can't read any settings from it then, so all you can do is a) keep it around for later or b) use it purely for writing out a new config file. Use case b) is no problem, so I think we're talking about use case a). Yes in theory an app could see a behavior change in that the config file is loaded from disk at the time of creating the skeleton rather than at the time of calling readConfig the first time. But this is why I'm making this change in 5.0 and not in 4.13. I think it's an acceptable behavior (matching KConfig's behavior more closely - it parses in the ctor, not in readEntry) and I doubt it affects many apps, since all I see everywhere is singletons - i.e. the KConfigSkeleton is created on demand at the time where it's first needed, therefore the ctor is immediately followed by readConfig.
My alternative idea (let's call it "3" to complete your list) was to pass a flag to the KConfig constructor to tell it "don't parse now", and setting that flag from the KConfigSkeleton constructor. Then readConfig can keep always calling reparseConfiguration(). This would work, right?
Your suggestion 1 is somewhat equivalent, but since one of the ctors for KCoreConfigSkeleton takes a KSharedConfig::Ptr as input, it's not applicable, we can't delay the creation of the kconfig within KCoreConfigSkeleton since it's created beforehand by the application.
Your suggestion 2 requires changing all the apps, which lowers the benefits of the optimization.</pre>
</blockquote>
<p>On March 1st, 2014, 6:08 p.m. UTC, <b>Matthew Dawson</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I agree, it is a weird use case, and the software should probably be adjusted. However, if an app does rely on that, it is very hard for the author of the software to notice the change, even with the port to 5. If I just looked at the functions names, I'd expect readConfig to read the file all the time. Following the principle of least surprise, I'd like to avoid readConfig ever not reading the file.
I'm fine with your alternate idea. I prefer that over my first idea, as it effectively does the same thing while being less invasive.
For my second suggestion, I realize its downsides. I just like following the principle of least surprise. If your alternate idea is implemented, I believe that would cover most cases. Suggestion 2 can then be implemented, and its related constructor could be marked deprecated. This would allow for existing programs to continue working, while allowing developers to change their code to take advantage of the optimization.
As I stated earlier, I'm not sure about who KDE wants to handle source compatibility with kdelibs. I also wouldn't mind just removing the second constructor, forcing all users to upgrade their code. Since the function is a drop in replacement, it wouldn't be that hard for developers to upgrade.</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I don't agree that "readConfig" necessarily implies reading from disk. All of the KConfig API says things like "readEntry", which does not read from disk, but uses the in-memory cache. Doing the same one level up, in KConfigSkeleton, would be rather consistent.
Your second suggestion is still a bit unclear to me; some function calling "new Configuration" (assuming that's the name of the derived class, like in kdnssd-framework/src/settings.h) is already what the generated code has with its self() method that takes care of calling the constructor anyway. So the app code is then just Configuration::self()->readConfig(); Configuration::someValue() (see e.g. kdnssd-framework/src/mdnsd-domainbrowser.cpp).
I'm not sure what you'd do in there to avoid the double parsing.
My own alternative idea leads to http://www.davidfaure.fr/2014/delayedparsing.diff which indeed helps less - still 3 parsings of oxygenrc, because of the three skeletons sharing the same KSharedConfig, and each of them calling readConfig. As you anticipated in your paragraph about RR #115964. What's your suggestion then for this issue? You suggest a named function - but changing names doesn't matter, this is called from generated code there too. See kde-workspace/libs/oxygen/oxygeninactiveshadowconfiguration.cpp (in the builddir) for instance.
My initial patch (the one in this RR) fixes that testcase fully because all skeletons share the same ready-to-use KSharedConfig, and none of them call reparseConfiguration in the first call to readConfig(). I.e. with sharing in mind, it makes more sense to say that it's the job of the KConfig to do the initial parsing than of KConfigSkeleton (of which there could be more than one). Of course this approach doesn't help with subsequent reparsings though (on a "config changed" signal) ... each readConfig() will trigger a full reparsing of the config file behind the scenes.... not great. It's starting to sound like we should offer a variant of readConfig() which just never calls reparseConfiguration(), i.e. leave that up to the application to deal with. At least for oxygen that would be perfect :-) For simple apps readConfig() would stay.
Conclusion: if you don't like this RR, I'm ok with http://www.davidfaure.fr/2014/delayedparsing.diff + a readConfig(NoReparse) or a separate method for which I can't find a good name right now.</pre>
<br />
<p>- David</p>
<br />
<p>On February 27th, 2014, 9:18 p.m. UTC, David Faure wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('https://git.reviewboard.kde.org/static/rb/images/review_request_box_top_bg.ab6f3b1072c9.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for KDE Frameworks and Matthew Dawson.</div>
<div>By David Faure.</div>
<p style="color: grey;"><i>Updated Feb. 27, 2014, 9:18 p.m.</i></p>
<div style="margin-top: 1.5em;">
<b style="color: #575012; font-size: 10pt;">Repository: </b>
kconfig
</div>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">KConfigSkeleton: avoid calling reparseConfiguration() immediately after creation.
KConfig already parses the config files from disk in the constructor,
which is necessary for non-KConfigXT users. However when using KConfigXT
the first thing one has to do after creation is to call readConfig(),
which should therefore not call reparseConfiguration the first time.
strace -e open kate |& grep -v NOENT | grep oxygenrc | wc -l
went from 4 to 1 --> bingo, goal reached!
(and when looking for kdeglobals, from 10 to 7)</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">(see commit log) + unittests in kconfig still pass.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>src/core/kcoreconfigskeleton.cpp <span style="color: grey">(9c5fb4a80d500e81b483b749a137ad5f2c99a55f)</span></li>
<li>src/core/kcoreconfigskeleton_p.h <span style="color: grey">(0b020ed3493186e699d872ddc7a9f9294d797a95)</span></li>
</ul>
<p><a href="https://git.reviewboard.kde.org/r/116461/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>