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

[charts] perf: getSymbol #15233

Merged
merged 2 commits into from
Nov 4, 2024
Merged

[charts] perf: getSymbol #15233

merged 2 commits into from
Nov 4, 2024

Conversation

romgrk
Copy link
Contributor

@romgrk romgrk commented Nov 2, 2024

From what I read in #15220, you seem to use this function in hot codepaths. It's going to hurt the performance to have that many memory allocations all over the place. Also the implementation has a small issue, indexOf(...) || 0 is incorrect, the empty value of indexOf() is -1.

I benchmarked a few different versions to illustrate the difference:

So even a change as small as moving the static line const symbolNames = 'circle cross diamond square star triangle wye'.split(/ /) to the outside of the function makes the function 2-3x faster, and diminishes the amount of time spent doing GC work.

@romgrk romgrk added performance component: charts This is the name of the generic UI component, not the React module! labels Nov 2, 2024
@mui-bot
Copy link

mui-bot commented Nov 2, 2024

Deploy preview: https://deploy-preview-15233--material-ui-x.netlify.app/

Generated by 🚫 dangerJS against 3172137

@romgrk romgrk requested a review from alexfauquette November 2, 2024 15:07
Copy link

codspeed-hq bot commented Nov 2, 2024

CodSpeed Performance Report

Merging #15233 will not alter performance

Comparing romgrk:perf-get-symbol (3172137) with master (1ca98d3)

Summary

✅ 3 untouched benchmarks

@JCQuintas
Copy link
Member

Nice catch. I saw this earlier and found it odd, but didn't check how often it was used 🤔

Thanks for taking care of it

@LukasTy
Copy link
Member

LukasTy commented Nov 4, 2024

Nice catch. 😱
Just for good measure, I've run the benchmark on my M1 Pro on Chrome:
Screenshot 2024-11-04 at 09 51 22

However, Safari shows more realistic numbers similar to the ones seen by Romaine:
Screenshot 2024-11-04 at 09 49 42

@romgrk romgrk merged commit 471ce00 into mui:master Nov 4, 2024
18 of 19 checks passed
@romgrk romgrk deleted the perf-get-symbol branch November 4, 2024 12:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
component: charts This is the name of the generic UI component, not the React module! performance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants