This revision was automatically updated to reflect the committed changes.
Closed by commit R244:78e9ffa1844e: Optimize inotify KDirWatch backend: map
inotify wd to Entry (authored by mwolff).
REPOSITORY
R244 KCoreAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9824?vs=25441&id=
mwolff added a comment.
In https://phabricator.kde.org/D9824#193095, @dfaure wrote:
> This patch fixes a O(n) performance issue in the inotify backend using a
cache, which makes KDirWatch *better* suited for applications that use
KDirWatch heavily. Clearly a step in the right direction.
rjvbb resigned from this revision.
This revision is now accepted and ready to land.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9824
To: mwolff, dfaure, #kdevelop, markg, aaronpuchert
Cc: aaronpuchert, bcooksley, zimmerman, markg, #frameworks
aaronpuchert accepted this revision.
aaronpuchert added a comment.
@rjvbb You certainly have a point when you say that there are more deficits
to address, but nothing will ever get done unless someone does it. Nobody here
is claiming that this solves all problems, but I don't see how it would
dfaure added a comment.
> What is slow in my experience is feeding a KDW instance.
That is part of the benchmark, see KDirWatch_UnitTest::benchCreateWatcher().
So the door is open for further improvements in that area.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.
rjvbb added a comment.
David Faure wrote on 20180119::08:33:32 re:
"https://phabricator.kde.org/D9824: Optimize inotify KDirWatch backend: map
inotify wd to Entry"
> Then why didn't you open XXX yourself to look for XXX like I did? It's
not rocket science, and
dfaure added a comment.
In https://phabricator.kde.org/D9824#193179, @rjvbb wrote:
> > That's not applicable.
>
> Did you read my actual, original request? If so you'd have seen that it's
satisfied by this answer.
Then why didn't you open kdirwatch.cpp yourself to look for
m_
rjvbb added a comment.
> That's not applicable.
Did you read my actual, original request? If so you'd have seen that it's
satisfied by this answer. Also please note the final remark in the author's
summary, `Note that the other backends could leverage a similar trick to speed
them up.`
markg accepted this revision.
markg added a comment.
Ship it from my side.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9824
To: mwolff, dfaure, rjvbb, #kdevelop, markg
Cc: aaronpuchert, bcooksley, zimmerman, markg, #frameworks
dfaure accepted this revision.
dfaure added a comment.
This patch fixes a O(n) performance issue in the inotify backend using a
cache, which makes KDirWatch *better* suited for applications that use
KDirWatch heavily. Clearly a step in the right direction. Yes a cache needs
memory, like all
rjvbb added a comment.
> Also we know at least one person that isn't going to forget about this,
don't we?
If you thought it'd be me *who* would keep nagging about this you can forget
about that.
> and you don't seem to dispute that it is an improvement
Because I simply don
aaronpuchert added a comment.
In https://phabricator.kde.org/D9824#192793, @rjvbb wrote:
> If real-world impact is in the order of a highly significant but
millisecond-order reduction of reaction time to file change there may be little
reason to commit this improvement now (and then risk
rjvbb added a comment.
I don't see anything unreasonable to my request, which can also be satisfied
by a argued demonstration why the same approach/fix doesn't apply to other
backends. AFAIAC I'm just doing my reviewers' service like it's been shown to
me how that's done.
If the same ap
bcooksley added subscribers: zimmerman, bcooksley.
bcooksley added a comment.
@rjvbb Please mind your language. From my perspective what you are asking of
@mwolff here is quite unreasonable - I can't see any reason why incremental
improvements, piece by piece would be unacceptable here.
REPO
rjvbb added a comment.
> i'm assuming the cost would be bigger then the benefit. (just an assumption
here, i could be wrong)
That's why it has to be measured.
I've never seen processing (reaction) times that measure in minutes with
either backend (which in turn is assuming that you
markg added a comment.
In https://phabricator.kde.org/D9824#191929, @rjvbb wrote:
> I am going to assume you would have said so by now if the fix is not
applicable to the QFSW backend because it doesn't do the same costly walk.
>
> > > So please MAKE your PATCH TOUCH the QFSW backen
rjvbb added a comment.
I am going to assume you would have said so by now if the fix is not
applicable to the QFSW backend because it doesn't do the same costly walk.
> > So please MAKE your PATCH TOUCH the QFSW backend AND MEASURE THE IMPACT
ON ITS PERFORMANCE
I don't care a rat'
mwolff added a comment.
In https://phabricator.kde.org/D9824#191748, @rjvbb wrote:
> So please do measure the impact of your fix on performance with the QFSW
backend.
It's zero, since this patch only touches the inotify backend.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
ht
rjvbb added a comment.
> But I won't write mega patches like you seem to prefer.
I don't. They happen when it's more cumbersome than self-evident to go back
and break up a new feature in baby-step components and justify adding them one
at a time.
> I try to keep things as minimal as
mwolff added a comment.
Yeah, I agree. Let's fix it. But I won't write mega patches like you seem to
prefer. I try to keep things as minimal as possible. And I also can't give you
an ETA or guarantee on when I'll fix the rest. This patch solves one big
issues, and the added benchmark lies th
rjvbb added a comment.
> @rjvbb why did you request changes to proceed with this patch? The fact
that there are more issues in KDirWatch does not mean we should hold up this
approach.
That should be obvious, no? You identified an issue but only addressed it in
a single backend. As far
mwolff added a comment.
@rjvbb why did you request changes to proceed with this patch? The fact that
there are more issues in KDirWatch does not mean we should hold up this
approach.
Also, please note how the fundamental issue described here affects more
backends beside inotify, but the
mwolff updated this revision to Diff 25441.
mwolff added a comment.
use QHash::insert instead of operator[]
REPOSITORY
R244 KCoreAddons
CHANGES SINCE LAST UPDATE
https://phabricator.kde.org/D9824?vs=25171&id=25441
REVISION DETAIL
https://phabricator.kde.org/D9824
AFFECTED FILES
src/
dfaure added a comment.
I'm 100% sure the same rules applies, because the "limitations" of operator[]
come from C++ and therefore apply to all associative containers.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9824
To: mwolff, dfaure, rjvbb, #kdevelop
Cc: m
markg added inline comments.
INLINE COMMENTS
> dfaure wrote in kdirwatch.cpp:721
> .insert(e->wd, e) is a hair faster, says Effective STL Item 24 :)
m_inotify_wd_to_entry is a QHash, not a std::unordered_map ;)
Not sure if the same rules from that book apply to Qt (i doubt it).
REPOSITORY
R24
mwolff added a comment.
150KB is not a lot of memory
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9824
To: mwolff, dfaure, rjvbb, #kdevelop
Cc: #frameworks
rjvbb added a comment.
> Each hash node will be approx 4+8=12 bytes, and something must point to it,
so another 8 bytes, 20 in total. No big deal.
I didn't think it'd be a big deal, not under normal circumstances. Still,with
applications like KDevelop that currently add every single item
dfaure added inline comments.
INLINE COMMENTS
> kdirwatch.cpp:721
> QFile::encodeName(e->path).data(), mask))
> >= 0) {
> +m_inotify_wd_to_entry[e->wd] = e;
> if (s_verboseDebug) {
.insert(e->wd, e) is a hair faster, says Effective STL Item 2
rjvbb added a comment.
Please test this for the QFSW backend too and post a benchmark result
comparison (and include the change if beneficial).
The QFSW backend is used on other Unices, Mac and on Linux when the inotify
backend cannot be used (NFS, for instance).
INLINE COMMENTS
> kdir
rjvbb requested changes to this revision.
rjvbb added a comment.
This revision now requires changes to proceed.
> @rjvbb this patch just shows that KDirWatch has tons of performance issues
that need to be fixed here.
That may be an open door but the plural of anecdote is not data. You fou
mwolff added a comment.
@rjvbb this patch just shows that KDirWatch has tons of performance issues
that need to be fixed here. Please do run the new benchmarks on your benchmark
with your backend, profile them, optimize them.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricat
mwolff added reviewers: rjvbb, KDevelop.
REPOSITORY
R244 KCoreAddons
REVISION DETAIL
https://phabricator.kde.org/D9824
To: mwolff, dfaure, rjvbb, #kdevelop
Cc: #frameworks
mwolff created this revision.
mwolff added a reviewer: dfaure.
Restricted Application added a project: Frameworks.
Restricted Application added a subscriber: Frameworks.
mwolff requested review of this revision.
REVISION SUMMARY
This greatly reduces the on-CPU time of the benchNotifyWatcher.
T
33 matches
Mail list logo
