Improve entropy recalculations #1879
Merged
+88
−6
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
nextstrain-bot
temporarily deployed
to
auspice-james-improve-e-3gx9pf
jameshadfield
force-pushed
the
james/improve-entropy-recalculations
branch
from
2c81abc to
4588d6c
Compare
jameshadfield
temporarily deployed
to
auspice-james-improve-e-3gx9pf
|
Updated to fix a bug when viewing narratives and also fix linting errors |
genehack
reviewed
Oct 28, 2024
jameshadfield
commented
Oct 28, 2024
jameshadfield
force-pushed
the
james/improve-entropy-recalculations
branch
from
4588d6c to
281bc90
Compare
jameshadfield
temporarily deployed
to
auspice-james-improve-e-3gx9pf
joverlee521
approved these changes
Oct 31, 2024
There was a problem hiding this comment.
I played around with the groups/blab/ncov-orf8ko/WA/20k dataset and definitely see improvements in responsiveness!
I only left non-blocking comments. In general, I find the split of management of entropy data between the Redux actions and Entropy component a bit weird. It feels like the Redux actions should trigger that entropy data updates appropriately and the Entropy component should just remain a consumer of that data.
jameshadfield
force-pushed
the
james/improve-entropy-recalculations
branch
2 times, most recently
from
65a982c to
446d5c9
Compare
jameshadfield
added a commit
that referenced
this pull request
Nov 5, 2024
joverlee521
approved these changes
Nov 5, 2024
by not recalculating data on the leading edge of the debounced recalculation action. For big trees (which take >500ms of time to redraw) the main thread is still blocked for roughly the same amount of time, but the tree is redrawn faster. For small trees which redraw quicker than that the entropy doesn't update until the debounce 500ms timeout is reached, resulting in slightly odd behaviour. (Reducing this timout also results in less-than-ideal behaviour as (e.g.) dragging the date range of a tree results in interruptions while the entropy calcs run which is worse IMO.)
Improves the performance of most interactions in Auspice when the `<Entropy>` panel's not displayed by not constantly updating the entropy calculations. The downside is when the panel's toggled on things are a little slower as we recalculate at that point. Note that the Entropy panel should never be rendered with invalid / stale / uncomputed data. Any action which results in the panel being shown is responsible for ensuring the underlying data is updated as appropriate.
Use an intersection observer to detect when the entropy panel is visible on the screen (viewport). When it's offscreen we don't update entropy data (an expensive calculation). When it comes back onscreen we recalculate the entropy data if it has become stale. This results in slightly strange behaviour when the entropy panel will be shown with no bars and they'll be drawn after a slight delay (while the data's recalculated). The wins are much improved performance when the entropy panel is not on screen (which is common).
jameshadfield
force-pushed
the
james/improve-entropy-recalculations
branch
from
3799a98 to
5543a86
Compare
jameshadfield
added a commit
that referenced
this pull request
Nov 12, 2024
The bug arose because window-resizes trigger a complete re-render of the entropy panel's SVG and this didn't correctly handle an edge case where the entropy data was stale (a concept introduced in #1879). For specific steps to reproduce see <#1895 (comment)> Closes #1895
This was referenced Nov 12, 2024
jameshadfield
added a commit
that referenced
this pull request
Nov 17, 2024
Changing between "Entropy - Events" results in two dispatches (`ENTROPY_COUNTS_TOGGLE` and `ENTROPY_DATA`) however the entropy code would ignore the first update if it didn't contain new entropy data. This bug has probably existed for the life of the entropy panel however was masked by those two actions happening back-to-back and being (always?) bundled together by redux. Recent work in #1879 (v2.60.0) meant these actions were no longer back-to-back and thus the bug was revealed. Closes #1905
jameshadfield
added a commit
that referenced
this pull request
Nov 17, 2024
This reverts commit 0c91724. That commit was the first performance optimisation within #1879. The idea was to skip the expensive recalculation when initially dragging the date slider. In other contexts this change was a negative, specifically when there was only one action (e.g. changing entropy to events) as we essentially introduced a 500ms delay. Note that when the panel is off-screen subsequent work in #1879 means that the recalculation is skipped anyway, so reverting this commit only affects performance when the panel is on screen and we're dragging the date slider. To remedy this we should add a code path which specifically skips the leading edge calculation when dragging the date slider. Essentially closes #1899 - that issue was partly the recalculation on-demand which remains, but greatly exacerbated by the unnecessary 500ms delay.
Merged
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.
Improves the general speed of Auspice when the Entropy panel is not displayed (either toggled off or off-screen). The downside is a small amount of time where there's a blank entropy panel when it scrolls back into view after a state change which requires the entropy data to be recalculated. (The win is that we skip these recalculations so everything else is faster.)
Eyes on the behaviour changes in this PR would be very welcome!
There's more we can do here, but I'll leave the following for subsequent PRs:
requestIdleCallbackAPIRelated to #1878