Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Improve tree performance for trees with over 4000 tips #1880

Merged
merged 2 commits into from
Nov 6, 2024

Conversation

jameshadfield
Copy link
Member

For big trees, attempting to animate changes results in poor performance
and we're better off making all SVG changes in a single pass. "big" here
depends on both the browser's performance and the size of the tree. Here
we just use the latter as it's simpler to code, but in the future we may
measure performance as the user interacts with the viz and update these
flags accordingly. (This is one of the reasons we calculate these in
middleware.)

For future, we should add nuance to this performance flag. For instance,
viewing a 2k tree and zooming into a clade has poor performance and we'd
be better off skipping the animation, however once in a small clade it
works well. For that same tree (viewing all 2k tips) filtering to a
clade still looks good with the animation.

4000 tips is arbitrary, it was chosen after testing a variety of
datasets locally.

Note that there is a big difference between not setting a transition
(within d3) and setting a transition of 0ms - the former is much faster.
See #1878 for more details
here. I didn't implement this change for modifySVGInStages as it uses
a more complex scheduling system; but the performance here is terrible
for really large trees so we should revisit this in future work.

Performance changes

Auspice built in production mode on Chrome 130.0 on a Apple M3 pro chip.
Datasets: a large ncov tree (23k tips) (see
#1878 for more), and a flu
tree of 2k tips. Both only rendering the tree panel.

Flu performance is unchanged, as 2k tips isn't enough to trigger this
performance toggle. For posterity:

  • Zooming into clade 2a. Initial frame ~160ms, then 30fps.
  • Filtering to clade 2a. Initial frame ~115ms, then 60fps.
  • Animation: 60fps

ncov performance:

  • Zoom into clade 21M.

    • Previously: Initial frame 1500ms, second (final frame) 430ms later,
      main blocked for a further 700ms
    • Now: Single frame of 921ms. Main blocked for a further 550ms.
    • Change: Time to correct tree reduced by ~1000ms (50%). Time until
      main is no longer blocked: reduced by ~1100ms (45%).
  • Filter to clade 21M.

    • Previously: Initial frame 1050ms, second frame 930ms later, main
      blocked for a further 660ms
    • Now: Single frame of 1018ms, main blocked for a further 660ms
    • Change: Time to correct tree reduced by ~950ms (50%). Time until
      main is no longer blocked: reduced by ~950ms (35%).
  • Animate. This is unchanged - Auspice sets the transition time to 0
    (see end of this commit message). We get around 4 frames at 70ms each
    (14fps) then a long commit phase blocking for ~650ms. For unknown
    reasons we trigger a timer flush but it doesn't show up in the
    performance snapshot, which is why these times are unchanged by this
    commit.

Note that the timings for the initial frame are slightly variable in
chrome. The JS time / blinkng pipeline parts shown on the main thread
seem pretty consistent, but when the frame is drawn (relative to when
the commit phase starts) is more variable. So take the above results
with a grain of salt.

P.S. there is a somewhat parallel implementation of this concept in
Auspice where PhyloTree sets a transition time of zero if subsequent
updates come in quick enough, but that is buggy. Firstly, if an update
takes a long time the main thread will be blocked so the next update
will be indistinguishable from one which was triggered an acceptable
time later. Secondly, it only works for animation where we have a stream
of updates.

src/middleware/performanceToggles.js Outdated Show resolved Hide resolved
src/middleware/performanceToggles.js Outdated Show resolved Hide resolved
Comment on lines 25 to 26
const totalTipCount = tree?.nodes?.[0]?.fullTipCount;
toggles.set("skipTreeAnimation", totalTipCount > 4000);
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Re: 0862223

For future, we should add nuance to this performance flag. For instance,
viewing a 2k tree and zooming into a clade has poor performance and we'd
be better off skipping the animation, however once in a small clade it
works well. For that same tree (viewing all 2k tips) filtering to a
clade still looks good with the animation.

I thought about something like tree.nodes.[tree.idxOfInViewRootNode].fullTipCount > 2000 but that would only work for half the time, e.g. from 4000 → 100 or 100 → 4000 but not both. This would have to be set somewhere like modifyTreeStateVisAndBranchThickness where both the old and new idxOfInViewRootNode are accessible and make the condition something like tree.nodes.[oldState.idxOfInViewRootNode].fullTipCount > 2000 || tree.nodes.[newState.idxOfInViewRootNode].fullTipCount > 2000.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In canonical redux this would be straightforward as middleware has access to the old state and because we use fat actions it also can see the new state. But as we modify the nodes directly (i.e. they're not immutable, largely for performance reasons) we can't leverage this. But we can see old and new idxOfInViewRootNode so we could implement the last bit of code you wrote within the middleware. (Would require quite a bit of testing!)

Copy link
Contributor

@joverlee521 joverlee521 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🚀

Performance flags (reduxState.controls.performanceFlags) represent
flags for which we enable/disable certain functionality in Auspice.
These flags shouldn't be depended on, i.e. Auspice should work just fine
without them (but may be a little slow).
For big trees, attempting to animate changes results in poor performance
and we're better off making all SVG changes in a single pass. "big" here
depends on both the browser's performance and the size of the tree. Here
we just use the latter as it's simpler to code, but in the future we may
measure performance as the user interacts with the viz and update these
flags accordingly. (This is one of the reasons we calculate these in
middleware.)

For future, we should add nuance to this performance flag. For instance,
viewing a 2k tree and zooming into a clade has poor performance and we'd
be better off skipping the animation, however once in a small clade it
works well. For that same tree (viewing all 2k tips) filtering to a
clade still looks good with the animation.

4000 tips is arbitrary, it was chosen after testing a variety of
datasets locally.

Note that there is a big difference between not setting a transition
(within d3) and setting a transition of 0ms - the former is much faster.
See <#1878> for more details
here. I didn't implement this change for `modifySVGInStages` as it uses
a more complex scheduling system; but the performance here is terrible
for really large trees so we should revisit this in future work.

Performance changes
-------------------

Auspice built in production mode on Chrome 130.0 on a Apple M3 pro chip.
Datasets: a large ncov tree (23k tips) (see
<#1878> for more), and a flu
tree of 2k tips. Both only rendering the tree panel.

Flu performance is unchanged, as 2k tips isn't enough to trigger this
performance toggle. For posterity:
* Zooming into clade 2a. Initial frame ~160ms, then 30fps.
* Filtering to clade 2a. Initial frame ~115ms, then 60fps.
* Animation: 60fps

ncov performance:
* Zoom into clade 21M.
  * Previously: Initial frame 1500ms, second (final frame) 430ms later,
    main blocked for a further 700ms
  * Now:  Single frame of 921ms. Main blocked for a further 550ms.
  * Change: Time to correct tree reduced by ~1000ms (50%). Time until
    main is no longer blocked: reduced by ~1100ms (45%).

* Filter to clade 21M.
  * Previously: Initial frame 1050ms, second frame 930ms later, main
    blocked for a further 660ms
  * Now: Single frame of 1018ms, main blocked for a further 660ms
  * Change: Time to correct tree reduced by ~950ms (50%). Time until
    main is no longer blocked: reduced by ~950ms (35%).

* Animate. This is unchanged - Auspice sets the transition time to 0
  (see end of this commit message). We get around 4 frames at 70ms each
  (14fps) then a long commit phase blocking for ~650ms. For unknown
  reasons we trigger a timer flush but it doesn't show up in the
  performance snapshot, which is why these times are unchanged by this
  commit.

Note that the timings for the initial frame are slightly variable in
chrome. The JS time / blinkng pipeline parts shown on the main thread
seem pretty consistent, but when the frame is drawn (relative to when
the commit phase starts) is more variable. So take the above results
with a grain of salt.

P.S. there is a somewhat parallel implementation of this concept in
Auspice where PhyloTree sets a transition time of zero if subsequent
updates come in quick enough, but that is buggy. Firstly, if an update
takes a long time the main thread will be blocked so the next update
will be indistinguishable from one which was triggered an acceptable
time later. Secondly, it only works for animation where we have a stream
of updates.
@jameshadfield jameshadfield force-pushed the james/improve-tree-perf branch from f09e0cb to 3a18d77 Compare November 6, 2024 22:16
@jameshadfield jameshadfield merged commit 2ae2280 into master Nov 6, 2024
26 checks passed
@jameshadfield jameshadfield deleted the james/improve-tree-perf branch November 6, 2024 22:26
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants