Skip to content

Stockcharts: Mock requestAnimationFrame to setTimeout so that the run…#176

Merged
bgrins merged 1 commit intoWebKit:mainfrom
mozilla:stabilize-react-stockcharts
May 31, 2023
Merged

Stockcharts: Mock requestAnimationFrame to setTimeout so that the run…#176
bgrins merged 1 commit intoWebKit:mainfrom
mozilla:stabilize-react-stockcharts

Conversation

@julienw
Copy link
Contributor

@julienw julienw commented May 23, 2023

…ner can catch asynchronous work

#97

current main branch
with this patch

I'm especially interested in the SVG version of the benchmark, and looking especially at the PanTheChart steps. Indeed these steps were the most unstable in the SVG version of the benchmark.

With this change I get very stable results in Firefox, and stable results in Chrome.

What do you think @rniwa @flashdesignory @bgrins?

Firefox Before/After
image
image

Chrome Before/After
image
image

@bgrins
Copy link
Contributor

bgrins commented May 23, 2023

I haven't checked other browsers but this pretty clearly moves work into React-Stockcharts-SVG.PanTheChart-async in Firefox (with PR: https://share.firefox.dev/3BTd1ni, without PR: https://share.firefox.dev/42YUSAs). And given that we're using this pattern for other workloads it seems like we should do it here, in addition to other updates to make the SVG workload more realistic like in #97.

@camillobruni
Copy link
Contributor

@rniwa rniwa added the non-trivial change A change that affects benchmark results label May 23, 2023
@julienw
Copy link
Contributor Author

julienw commented May 24, 2023

I tested with your rAF-based patch. This seems to change things but isn't removing the problem completely:

  • When not profiling, I still see the problem in the reported metrics.
  • When profiling, the problem doesn't seem to be there anymore.

So there might still be subtle racing problems that I don't fully understand, even with your patch.

@camillobruni camillobruni self-requested a review May 25, 2023 07:53
@julienw julienw force-pushed the stabilize-react-stockcharts branch from 84bd558 to 7a4339a Compare May 25, 2023 09:44
@julienw
Copy link
Contributor Author

julienw commented May 25, 2023

I rebased on top of the latest main branch and included the results in Firefox and Chrome in the PR description above.

@bgrins bgrins merged commit 4f9ffb5 into WebKit:main May 31, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

non-trivial change A change that affects benchmark results

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants