Conversation
This file contains hidden or 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
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.
Closes #5927
Closes #5790
Closes #8036
(Supersedes #7998)
Fix unreasonably slow scattergl hover behavior (to the point of freezing) in the default case. Even with this fix, the hover is still somewhat laggy, but doesn't freeze entirely as in the linked issues.
The scattergl hover logic was following a code path which looks for nearby points to draw spikelines to. This code path was being followed even when spikelines were not enabled. The search is very slow with large numbers of points. This PR adds a guard to skip the "find nearby points" search unless required.
Note: For the diff chunk in
src/components/fx/hover.jsstarting on line 671, the only meaningful change is addingcanSpikeToNonHoveredPoint()to theifcondition. The rest of that diff is just indentation change from collapsing two nestedifstatements into 1.Screen recording
This video shows the performance I see on my machine after this fix, with extremely closely-spaced points.
scattergl-hover.mov
Steps for testing:
main. Notice that the entire plot freezes completely when trying to hover.Caveat
There's an underlying performance issue which is not addressed by this PR, which probably deserves its own issue. #8036 points out that there's a performance cliff which starts at 100k points, even though 99,999 points are fine. At 100k points we exceed the
TOO_MANY_POINTSconstant defined insrc/traces/scattergl/constants.ts, which triggers us to use a tree data structure for searching through points, rather than a plain array.I haven't fully investigated but there seems to be some performance issues with that tree implementation (which is ours: see https://github.com/plotly/point-cluster), specifically with the
.range()function called here. It's extremely slow when given infinite bounds, and seems to be slower than regular array traversal whenever the bounds cover a significant fraction of the total points. Either we're calling the.range()function in a non-optimal way, or there's a bug in the implementation. Hence why even with this fix, the scattergl hover is still somewhat laggy as soon as we exceed 100k points.