Live viewer: Quick handle notification and defer slow work #2221
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
JackEAllen
requested changes
Jun 13, 2024
There was a problem hiding this comment.
Overall, looks good, thanks for chasing down this bug and putting a measure in to better handle the build up of queued signals. The implementation of _handle_notified_of_directry_change looks good, just a small typo in a docstring.
The main bit of feedback I have is just to move scripts/simulate_live_data_stress.py into scripts/simulate_live_data.py. This would not only keep the number of utility scripts we have small, but also improve usability of scripts/simulate_live_data.py by no longer requiring users/devs from needing to enter a source directory if we draw an image if no source is provided.
Handle directory changed events quickly by storing the directory and starting a timer (if not already started). Trigger the slow part of the work with the timer. This means multiple changes can be flushed off the Qt signal queue, and the slow part done once.
samtygier-stfc
force-pushed
the
2204-fast-handle-changed
branch
from
810eee1
to
7b5f142
Compare
JackEAllen
reviewed
Jun 17, 2024
JackEAllen
approved these changes
Jun 17, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Issue
Closes #2204
Description
When a diectory is updated we have to check the age of every file in it, to find the newest (QFileSystemWatcher does not give the changed file name). This can be slow on windows, e.g. a second for a few thousand images. During this time new notifications can arrive.
Previously, each notification as acted when it was received. If the rate of notification was to high, then a queue of signals would build up, and MI would appear stuck.
Now we can quickly handle the notification in
_handle_notified_of_directry_change(), and defer the work with a timer. Any extra notifications can get aborbed by this, and the actuall work in_handle_directory_change()only needs to run once.Also adds a stress test script. e.g.
simulate_live_data_stress.py -d \mi_test\live -s 0 -f 1000 -r 0.1 -vCreates 1000 files at a rate of one every 0.1 seconds.
Testing
Not easy to add an automated test
Acceptance Criteria
See bug report for methods to trigger issue. Check that you can trigger it on main. Test that there are not long hangs with changes.
Not that this does nothing to make finding the new file faster. So with a large number of files there will still be a few seconds between a file arriving and being displayed.
Documentation
release notes