> 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
>
>
_______________________________________________
Kde-frameworks-devel mailing list
Kde-frameworks-devel@kde.org
https://mail.kde.org/mailman/listinfo/kde-frameworks-devel
