> On June 28, 2014, 11:31 p.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.
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.
- Matthew
-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/118985/#review61157
-----------------------------------------------------------
On June 29, 2014, 9:55 a.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, 9:55 a.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
