feat(track): performance improvements #988
Conversation
etowahadams
changed the title
etowahadams
changed the title
|
Zooming works (after much trial and error). However it is still more choppy than I would like. Not sure that there's an obvious solution. |
etowahadams
changed the title
|
For reasons still unclear to me, it seems that occasionally the first zoom/pan event has a lower frame rate compared to subsequent interactions. |
|
Last thing to do here is just to add the same visual regression testing from #962 |
|
/update-snapshots |
1 similar comment
|
/update-snapshots |
|
test |
|
/update-snapshots |
|
I spent quite a bit of time getting automatic snapshot testing working in CI, but without luck. Maybe it needs to be merged before it can be triggered from a comment? Will have to do additional testing. Going to reduce the scope of this to just adding the performance test and the performance related changes. |
|
|
||
| this.specOriginal = JSON.parse(JSON.stringify(spec)); | ||
| this.specComplete = JSON.parse(JSON.stringify(spec)); | ||
| this.specOriginal = spec; |
There was a problem hiding this comment.
specOrigional never gets modified so we don't need to make a clone of it
| getResolvedTracks() { | ||
| if (!this.resolvedTracks) { | ||
| const copy = structuredClone(this.options.spec); | ||
| const tracks = resolveSuperposedTracks(copy).filter(t => t.mark !== 'brush'); | ||
| // We will never need to access the values field in the data spec. It can be quite large which can degrade performance so we remove it. | ||
| tracks.forEach(track => { | ||
| if ('values' in track.data) { | ||
| track.data.values = []; | ||
| } | ||
| }); | ||
| this.resolvedTracks = tracks; | ||
| } |
There was a problem hiding this comment.
Rather than creating a resolved spec every time, we make it once and reuse it.
There was a problem hiding this comment.
I tried to come up with ways in creating a new resolved spec at every render is needed but couldn't think of one. Sehi do you know if there are any?
There was a problem hiding this comment.
It is possible that the track spec can be edited by users even after the track has been rendered, so we would need to force updating resolvedTracks when this happens. override rerender(newOptions: GoslingTrackOptions) is the function that is called when a user edits a track spec, so we can update resolvedTracks inside the function.
I don't think this will decrease the performance though since this function is called only when the user manually edits the spec.
There was a problem hiding this comment.
That's exactly what I was looking for, thank you! Just changed.
| // We will never need to access the values field in the data spec. It can be quite large which can degrade performance so we remove it. | ||
| tracks.forEach(track => { | ||
| if ('values' in track.data) { | ||
| track.data.values = []; | ||
| } | ||
| }); |
There was a problem hiding this comment.
I reviewed the code changes mainly related to the renderer, i.e., changes in gosling-track.ts and gosling-track-model.ts, and left one comment to force updating resolvedTracks when a user manually edits the spec. I will need to find more time to look at other updates (playwright), but please feel free to merge this after addressing the comment.
Fix #
Toward #
Change List
GoslingTrackClassso that it doesn't have to be regenerated every timeChecklist