perf(sync): remove 3ms scheduling delay between propagation jobs#9992
perf(sync): remove 3ms scheduling delay between propagation jobs#9992qoole wants to merge 1 commit into
Conversation
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
can you add some benchmark in the test folder to prove that we would indeed really gain the 30 seconds for 10 000 files ?
I would need to be convinced
you also need to split unrelated changes in different commits with proper commit messages ?
the change of the buffer size is unrelated to the other change
scheduleNextJob() deferred the next scheduling pass with QTimer::singleShot(3, ...). The _jobScheduled flag already guarantees only one scheduling pass is pending at a time, so the 3ms delay served no purpose other than throttling. Because the flag serializes passes, each of the ~N scheduling passes for a sync of N files paid the full 3ms latency back-to-back; for 10,000 small files that is ~30 seconds of pure scheduling latency. Replace it with QMetaObject::invokeMethod(Qt::QueuedConnection), which defers to the next event-loop iteration (preserving re-entrancy safety) without the artificial delay. Signed-off-by: Qoole <2862661+qoole@users.noreply.github.com>
qoole
force-pushed
the
perf/sync-scheduling
branch
from
c0615ec to
caabebc
Compare
|
Addressed both points. 1. Split unrelated changes into separate commits:
2. Benchmark for the 30 s claim. Added So ~34 s of pure scheduling latency is removed for a 10,000-file sync, confirming the original estimate. The benchmark is runnable in CI so you can verify the delta independently. |
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
the new benchmark is irrelevant and is not enough to validate your change
you should have a benchmark running the real synchronization code and show that there is a real significant delay without the change in this PR
mgallien
left a comment
mgallien
left a comment
There was a problem hiding this comment.
can you change the PR to only include the changes for QTimer::singleShot ?
I did some manual investigation and this change is good enough for merge
thanks
|
Artifact containing the AppImage: nextcloud-appimage-pr-9992.zip Digest: To test this change/fix you can download the above artifact file, unzip it, and run it. Please make sure to quit your existing Nextcloud app and backup your data. |
|
@qoole gentle ping |
qoole
force-pushed
the
perf/sync-scheduling
branch
from
caabebc to
27e80d7
Compare
qoole
changed the title
|
Trimmed to only the |
2 participants
Summary
scheduleNextJob3ms timerOwncloudPropagator::scheduleNextJobschedules the next call viaQTimer::singleShot(3, ...). The 3ms delay was originally there to debounce re-entry, but the_jobScheduledflag at the top of the function already prevents that — once set, repeat calls return immediately.Replaced with
QMetaObject::invokeMethod(Qt::QueuedConnection)which posts to the event loop with no artificial delay.With 10,000 small files this delay accumulates to roughly 30 seconds of scheduling latency that is pure idle time on the event loop. On a sync of many tiny files this is the dominant cost of the propagator.
Checklist
AI (if applicable)