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

perf(track): Persist GoslingTrackModels #1044

Merged
merged 2 commits into from
Feb 22, 2024
Merged

Conversation

etowahadams
Copy link
Contributor

Fix #1043
Toward #

Change List

  • Persist GoslingTrackModels between draw calls
  • Switch from structuredClone() to JSON.parse(JSON.stringify())
Screen.Recording.2024-02-22.at.9.47.14.AM.mov

Checklist

  • Ensure the PR works with all demos on the online editor
  • Unit tests added or updated
  • Examples added or updated
  • Documentation updated (e.g., added API functions)
  • Screenshots for visual changes (e.g., new encoding support or UI change on Editor)

@etowahadams
Copy link
Contributor Author

Persisting GoslingTrackModels is something I originally tested in #1039. This offers substantial performance improvements since now the GoslingTrackModels don't have to be regenerated with every draw() call.

It does seem that JSON.parse(JSON.stringify()) is about 40% faster than structuredClone() in Firefox so I switched it back to that https://www.measurethat.net/Benchmarks/ShowResult/506888

@etowahadams etowahadams requested a review from sehilyi February 22, 2024 15:04
Comment on lines +562 to +566
// If we have already processed all tiles, we don't need to do anything
// this.#processedTileMap contains all of data needed to draw
if (tiles.every(tile => this.#processedTileMap.get(tile) !== undefined)) {
return;
}
Copy link
Member

Choose a reason for hiding this comment

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

This is nice! I assume this also increases the performance to some extent.

Copy link
Member

@sehilyi sehilyi left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks a lot for such a quick fix!

@sehilyi
Copy link
Member

sehilyi commented Feb 22, 2024

JSON.parse(JSON.stringify()) being faster than structuredClone() is surprising. I thought it was the opposite.

@etowahadams
Copy link
Contributor Author

Its worth noting that this reduces, but doesn't completely get rid of the freezing that sometimes occurs when zooming and panning. As far as I can tell, the reason for this is because of the HiGlass renderer, where an infinite loop can happen. The renderer detects that the scales have changed before the drawing has finished so it tries redrawing everything according to the new scales. But after it redraws, it sees that the scales have been updated again so it draws everything again.

image

@etowahadams etowahadams merged commit 560d121 into main Feb 22, 2024
4 checks passed
@etowahadams etowahadams deleted the etowahadams/zoom-pan branch February 22, 2024 15:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Synchronous navigation of tracks seems broken
2 participants