> 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. > > David Faure wrote: > 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) > > Matthew Dawson wrote: > I'm not sure about loadConfig, as it kinda of implies loading from disk, > so I think the best solution would be rewriting history :) > > Instead, maybe the best is to use use loadConfig to mean reading from > disk. So the matrix looks like: > > * kconfigskeleton: load(Config) = from disk, parse(Config) = in memory > * kconfig: (re?)load(Configuration) = from disk, read(Entry) = > memory -> though maybe even change this to parseEntry? > > Assuming readEntry stays the same, the load* functions are just a rename. > So the old names can be marked as deprecated, and the documentation points > to the names. SC/BC is simple, since the one function just calls the other. > And people still using readConfig get a huge warning about the fact they need > to change to parseConfig. > > For more symmetry, readEntry can change to parseEntry, with a similar > deprecation. I'm not sure this is really necessary, as parseConfig is > different from readEntry. Also, this change can be done separately, so it > shouldn't hold up the more useful optimization. > > Alternate thought: why are the methods even named readEntry? That > implies they actually parse the data returned. They could just as easily be > named fetchEntry, getEntry, or just entry. writeEntry is the same, it > doesn't actually hit the disk, so it could be put* or set*. Thoughts? > > David Faure wrote: > I like load(). Anyone seeing two calls to load() on the same object will > wonder about the possible bad effect on performance, since it sounds heavy :) > > Renaming readEntry: veto! This is the most called method of them all, the > porting effort would be huge, just to make the two of us happy with naming :-) > And read/write from memory doesn't seem so crazy (QBuffer, QMetaProperty, > Q*Item etc. do this too). > > Overall I like your suggested matrix, but now we should look at > minimizing the porting. > > KConfig::reparseConfiguration() wasn't misleading, before KConfigXT. The > only reason for "reparsing" is reading the ini file on disk an updating the > in-memory cache. However the problem with KConfigSkeleton is that it's one > layer on top of KConfig. So parsing/loading there is all about getting stuff > from KConfig. So yeah, to align naming, I could see loadConfiguration() [Qt > says: no abbreviations] in both layers for "loading from disk". (Not > "reload", given the new Delayed flag (to be renamed to DelayedLoading), in > which case this would also be about the initial loading from disk). > > It just occured to me that all this "Config" or "Configuration" in the > method names is rather useless, this is about KConfig* anyway. > So how about just load()? > > With all this in mind my current suggestion for a game plan would be: > 1) New method KConfigSkeleton::read() for reading from KConfig without > loading (i.e. without disk usage) > 2) KConfigSkeleton::readConfig() deprecated for new name > KConfigSkeleton::load() > 3) KConfig::reparseConfiguration() deprecated for new name KConfig::load() > > What do you think? > > > Matthew Dawson wrote: > I agree the porting effort for renaming readEntry would be huge. I just > threw it out there as it popped into mind. > > I like your final game plan, so I think it is the best path forward. I > just have a couple of additions. > > Since we are going down the renaming path, I was thinking of the write > side. I think it be good to standardize the naming. Right now KConfig uses > sync(), while KCoreConfigSkeleton uses writeConfig(). Since writeEntry() > exists, and is the larger porting effort, I think both should become sync(). > So, the game plan would get: > > 4) Deprecate KCoreConfigSkeleton::writeConfig() for the new name > KCoreConfigSkeleton::sync(). > > Also, don't forget the usr*Config functions. I don't think they should > just be renamed, as quite a bit of software relies on them. I think > replacement functions could be created, and those replacements call the > originals. So, also add: > > 5) Deprecate KCoreConfigSkeleton::usr*Config() for > KCoreConfigSkeleton::usrLoad()/KCoreConfigSkeleton::usrSync(). > > David Faure wrote: > Hmm, sync() could sound a bit like what load() does, i.e. "the file > changed on disk, sync the kconfig object with it, i.e. load it". But of > course historically it didn't mean that. > > Since it's the opposite of load(), naming it save() would have seemed > much more intuitive to me. > > Especially since (see documentation), while it merges with independent > changes on disk, it doesn't actually load these changes into the object. So > it's really just a save(), not a sync=save+load. > > In suggestion 5, you mean adding non-virtual forwarding methods? But > these are protected virtual methods, so it would be pointless. IOW the only > API there is "what should I reimplement", which can only be one or the > other... > > > > Matthew Dawson wrote: > I choose sync for similarity, and to avoid porting work. In KConfig, > sync() doesn't re-read anything, so I agree that calling it save() makes more > sense. I'd even prefer to rename them. Unforunately, lxr makes it hard to > find a re-implementation of sync (since it is virtual). It probably wouldn't > be too bad to deprecate and change over to save() though. > > Oddly enough, KCoreConfigSkeleton does in fact do a read after the write. > I can't find out why, as I traced the code back to when it was committed, > and it was just part of the file with no rationale given. I want to > investigate that, as it seems pointless. > > I meant to rename the usr*Config functions. In the interest of > maintaining SC/BC, I was thinking to also keep the original methods. I'd be > just as happy to kick them to the curb, but they have some significant usage > according lxr.kde.org. Keeping them around would at least allow for porting > to be easier. > > David Faure wrote: > Replying in order: > > In all my years in KDE I have never seen someone reimplement > KConfig::sync() indeed. We could de-virtualize it. > > Read after write pointless? Well, the values have been updated on disk > and are available, we might as well use them? Not 100% sure. It's true that > in the cases where we care for showing changes by other processes, we ensure > there's some notification anyway, in which case we reload anyway > (independently from the saving code). > > One thing I realized when writing 116845: the name of the virtual method > usrReadConfig() is just fine. We should NOT rename that to usrLoad() since > it's called by read(), not only by load(). load() -> read() -> > usrReadConfig(). > > > Matthew Dawson wrote: > In order as well: > > Looking over how the classes behave, I believe it exists as a virtual so > that KConfig and KConfigGroup can both behave as KConfigBase, allowing for > software to take a KConfigBase, and store their settings in there, along with > request it to save. So renaming sync() -> save() should be safe, and a > compatibility save() can be kept in KConfigBase, keeping SC. > > Unless the values have changed on disk, then saving should mean > re-reading the values will produce the exact same result. The only values > that can change are other un-modified values. The api documentation also > doesn't mention this. I rather have a sync() function that writes and the > reads, as save() doesn't imply there will be a read. At least for now, > removing the extra read would be good. If users end up often doing a > save+load, a simple sync() function can be added for that case. > > Oops, I meant to rename it to usrRead(), not usrLoad().
OK for sync() -> save(), I'll make another RR (virtual method, so this is urgent). I don't see any need for save+load, as I said, because any app whose config might get changed externally (and who cares about it) already has to reload on changes, so this is handled before saving time. usrReadConfig() -> usrRead() seems like too big a SC (given that it's a virtual method meant to be reimplemented) to me. Apps don't use Q_DECL_OVERRIDE right now, so they'll get bitten. - 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
