> On Feb. 28, 2014, 8:41 p.m., Matthew Dawson wrote: > > 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. > > David Faure wrote: > 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. > > Matthew Dawson wrote: > 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. > > David Faure wrote: > 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. > > Matthew Dawson wrote: > While readConfig doesn't necessarily imply reading values from disk, the > documentation specifically states that it does ( > http://api.kde.org/frameworks-api/frameworks5-apidocs/kconfig/html/classKCoreConfigSkeleton.html#a3772e0cd58790c312b8d0802bc78a70c > ). As we both agree, most applications don't care about a change in > behaviour here. I just worry some might in a subtle way that is hard to > detect with this change. If readConfig broke in obvious way because of this > change, I wouldn't care as developers would know to make this change. > > For my second suggestion, I'm basically suggesting a static function that > looks like: > template<typename T> > T *makeConfigSkeletonWithoutReparse<T>(KSharedConfig::Ptr ptr) > { > T *obj = new T(ptr); > obj->privateReadConfigWithoutReparse(); > > return obj; > } > The oxygen code can just call this function to get the right object back, > and the privateReadConfigWithoutReparse will avoid the reparseConfiguration > call. Generated code from kconfig_compiler can easily use this for > construction, and projects can automatically pick up the enhancement. It > would be similar to using the KConfig::openConfig call. > > I'd be fine with an alternate readConfig (name suggestion: parseConfig, > thus (re-)parsing the given KConfig). Then my second suggestion can just go > away. I do worry that readConfig is currently a virtual, and thus > parseConfig won't make use of it. Probably best would be to make readConfig > non-virtual, and make it basically call KConfig::reparseConfiguration, then > call parseConfig. Then users just override parseConfig instead. Code make > break because of this, but any code that moves to using c++11's override will > break in an obvious way. Any other code will break if readConfig is called > through virtual dispatch, so it should be relatively obvious. Add some > documentation updates, and it I think it will go over ok. > > For delayedparsing.diff, can you throw it up on RB? I'd like to get it > in too, but I have a few thoughts on it and I rather do that in a separate > request.
I don't like the template-factory-method, it makes for horrible API IMHO :) But I love your parseConfig() suggestion, so I shall go ahead an implement it. It is much better because it fixes two issues: the N reparse on startup when sharing the same config object between N skeletons, but *also* the N reparse whenever we are notified that the config changed. (all my patches until now didn't fix that). If e.g. oxygen takes care of calling reparseConfiguration by itself upon a change, then it can simply call parseConfig in all skeletons. The one thing that might be confusing is the naming: * in kconfigskeleton, read(Config) = from disk, parse(Config) = in memory * in kconfig, read(Entry) = in memory, reparse(Configuration) = from disk !!! If only I could rewrite the past, I would swap that in kconfigskeleton. Alternatively, it's probably better to find another name for the new method... loadConfig? (as in, loading from kconfig to skeleton members) - David ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://git.reviewboard.kde.org/r/116461/#review51374 ----------------------------------------------------------- On Feb. 27, 2014, 9:18 p.m., David Faure wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://git.reviewboard.kde.org/r/116461/ > ----------------------------------------------------------- > > (Updated Feb. 27, 2014, 9:18 p.m.) > > > Review request for KDE Frameworks and Matthew Dawson. > > > Repository: kconfig > > > Description > ------- > > 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) > > > Diffs > ----- > > src/core/kcoreconfigskeleton.cpp 9c5fb4a80d500e81b483b749a137ad5f2c99a55f > src/core/kcoreconfigskeleton_p.h 0b020ed3493186e699d872ddc7a9f9294d797a95 > > Diff: https://git.reviewboard.kde.org/r/116461/diff/ > > > Testing > ------- > > (see commit log) + unittests in kconfig still pass. > > > Thanks, > > David Faure > >
_______________________________________________ Kde-frameworks-devel mailing list Kde-frameworks-devel@kde.org https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
