Skip to content
This repository has been archived by the owner on Jun 4, 2024. It is now read-only.

Reduce downsamples and re-renders for plot #7362

Conversation

defunctzombie
Copy link
Contributor

@defunctzombie defunctzombie commented Jan 22, 2024

User-Facing Changes
Plot performance improvements

Description
Prior to this change, any plot panel would re-downsample its dataset on any player state update regardless of whether it had new data to render or not. This change updates the handlePlayerState method of IDatasetsBuilder to return the number of datums added in addition to the range. The coordinator can avoid downsampling if there are no datums read.

This change also splits the downsample dispatch from the rendering of the downsampled datasets. Since these are powered by separate workers we can start work on a new downsample request while rendering the old.

@defunctzombie
Copy link
Contributor Author

@jtbandes @snosenzo I have to port the remaining DatasetsBuilders to the new interface but I wanted to get your first impressions of this approach for some light optimization to reduce plot rendering. It felt natural to return the number of datums that would be added via the handlePlayerState call but maybe you will see some other way that feels nicer.

I could also see a path here where the datasets builder dispatches calls to render itself with the latest viewport or downsampled datasets. That would require changing how the communication happens but would avoid a hop through the main thread. I see this change as a small improvement towards that larger change - but if we want to tackle the larger change I can see that happening too.

Copy link
Contributor

@snosenzo snosenzo left a comment

Choose a reason for hiding this comment

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

a little worried about the passing of mutable bounds references that are being set to private member variables, but otherwise looks good


// If the range has changed we will trigger a render to incorporate the new range into the chart
// axis
if (!_.isEqual(this.#datasetRange, newRange)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Are there any concerns for floating point precision issues that should be taken into account when comparing bounds? Should we give a threshold of equality?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good call-out but in this case I am ok with the strict equality. What I am checking for is whether the exact values are already set on #datasetRange. Any deviation and I do want to update the coordinator's range.

@defunctzombie defunctzombie merged commit 2841de2 into main Jan 30, 2024
15 checks passed
@defunctzombie defunctzombie deleted the roman/fg-6309-avoid-dataset-re-downsample-when-playing-if-no-dataset branch January 30, 2024 21:03
defunctzombie added a commit that referenced this pull request Jan 31, 2024
defunctzombie added a commit that referenced this pull request Jan 31, 2024
Reverts #7362

It broke the x-axis window mode.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants