Review Request 118985: KSharedConfig: move mainConfig and wasTestEnabled to the thread storage.

[David Faure]

> On June 29, 2014, 3:31 a.m., Matthew Dawson wrote:
> > I'm not sure about moving the warning about immutable files to only happen on the main thread, as it is possible that an application may use it KSharedConfig on an alternate thread only.
> > 
> > As this is mainly an optimization, I think the best thing is to just move to using the load() function on the atomic integer, as I believe that doesn't require processors to synchronize, and won't limit optimizations.  Otherwise, I'm fine with the immutable files as that is simpler code wise.  Thoughts?
> 
> David Faure wrote:
>     Well, I'm not sure we'd expect secondary threads to pop up a dialog though (as KConfig::isConfigWritable(true /*warn user*/) does, via QProcess("kdialog")).
>     
>     This bit (the warnUser code) isn't really about an optimization, but about doing that stuff only in the main thread, where GUI apps initialize the main config object and where user-interaction is expected.
> 
> Matthew Dawson wrote:
>     I wouldn't either, and I'd except most code access its main config on a primary thread first.  I'm just thinking for those 1% of cases, that cause all the trouble.
>     
>     When I mentioned about an optimization, I meant is that the reason for this RR, not the point of the user warning.  With the relaxed loading of the atomic int, I don't think there is much, if any of a performance hit.  I don't think the code complexity is too bad to handle that 1% either.  Is there any other reason for this change?  Otherwise, I'd prefer to be safe.
>     
>     Otherwise, this patch looks good with the latest changes.

This patch wasn't initially about optimization, but about fixing the behavior for the main thread vs other threads. I think the final version of the patch makes sense:
* caching, including mainConfig -> for all threads
* clearing cache when qstandardpaths test mode gets enabled -> for all threads because it's related to the above, even if unlikely to happen in other threads
* warning the user with a kdialog -> main thread, because this is a GUI operation, which is only expected in the main thread (even if it's technically possible to have it in other threads, since it's delegated to a separate process).

This patch is definitely not about improving performance, but about correctness :)


- David


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


On June 29, 2014, 1:55 p.m., David Faure wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/118985/
> -----------------------------------------------------------
> 
> (Updated June 29, 2014, 1:55 p.m.)
> 
> 
> Review request for KDE Frameworks and Matthew Dawson.
> 
> 
> Repository: kconfig
> 
> 
> Description
> -------
> 
> KSharedConfig: move mainConfig and wasTestEnabled to the thread storage.
> 
> This enables the mainConfig optimization in all threads,
> and ensures the user warning only happens in the main thread.
> 
> The test-mode-enabled logic is only really useful in the main thread,
> but it's simpler to just do it in all threads.
> 
> REVIEW: 118985
> 
> 
> Diffs
> -----
> 
>   autotests/kconfigtest.cpp a8482b7099df5921909830082d758dc3095e3241 
>   src/core/ksharedconfig.cpp b7d155d5893502921d35d7dd971188b6a93a0620 
> 
> Diff: https://git.reviewboard.kde.org/r/118985/diff/
> 
> 
> Testing
> -------
> 
> no regression in "make test" in kconfig; still debugging races in helgrind threadtest (in kio).
> 
> 
> Thanks,
> 
> David Faure
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kde-frameworks-devel/attachments/20140630/67e3cea0/attachment-0001.html>