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

Wrap BaseIdent name_override_opt & TraitName in an Arc #5978

Merged
merged 5 commits into from
May 9, 2024

Conversation

JoshuaBatty
Copy link
Member

@JoshuaBatty JoshuaBatty commented May 9, 2024

Description

I generated a flame graph and saw that we are spending a ton of time cloning BaseIdent and TraitName. I've wrapped the name_override_opt in BaseIdent with an Arc and also changed TraitName to:

type TraitName = Arc<CallPath<TraitSuffix>>;

Again running the same test being used in #5976 and also incorporating that PR's optimisations for the benchmark, we are getting a further 33.47% performance increase.

Before: Elapsed time: 22.02603425s
After: Elapsed time: 14.653166459s

In discussions with Jibril today, we should probably look at implementing a String interning system but this can be addressed in a future PR.

@JoshuaBatty JoshuaBatty self-assigned this May 9, 2024
@JoshuaBatty JoshuaBatty added the compiler General compiler. Should eventually become more specific as the issue is triaged label May 9, 2024
Copy link

github-actions bot commented May 9, 2024

Benchmark for 0448ab5

Click to view benchmark
Test Base PR %
code_action 5.6±0.10ms 5.4±0.10ms -3.57%
code_lens 300.5±9.76ns 288.3±8.68ns -4.06%
compile 6.7±0.08s 3.9±0.05s -41.79%
completion 5.1±0.03ms 4.7±0.01ms -7.84%
did_change_with_caching 6.1±0.04s 3.4±0.04s -44.26%
document_symbol 957.3±16.36µs 977.2±30.66µs +2.08%
format 87.6±1.68ms 90.4±1.28ms +3.20%
goto_definition 343.3±9.09µs 368.3±7.64µs +7.28%
highlight 9.1±0.14ms 9.0±0.17ms -1.10%
hover 621.2±8.36µs 486.9±4.06µs -21.62%
idents_at_position 118.6±0.65µs 122.6±1.19µs +3.37%
inlay_hints 653.4±9.43µs 665.4±22.74µs +1.84%
on_enter 511.8±22.42ns 482.2±15.24ns -5.78%
parent_decl_at_position 3.7±0.04ms 3.7±0.03ms 0.00%
prepare_rename 345.8±10.35µs 367.8±6.67µs +6.36%
rename 9.6±0.03ms 9.6±0.19ms 0.00%
semantic_tokens 1066.6±14.07µs 1066.1±64.91µs -0.05%
token_at_position 339.3±2.87µs 357.4±2.16µs +5.33%
tokens_at_position 3.7±0.02ms 3.7±0.03ms 0.00%
tokens_for_file 417.8±4.17µs 424.1±1.88µs +1.51%
traverse 51.1±2.00ms 40.2±1.09ms -21.33%

@JoshuaBatty JoshuaBatty requested review from a team May 9, 2024 18:39
@JoshuaBatty JoshuaBatty enabled auto-merge (squash) May 9, 2024 18:40
@JoshuaBatty JoshuaBatty merged commit 76c41c8 into master May 9, 2024
38 checks passed
@JoshuaBatty JoshuaBatty deleted the josh/call_path branch May 9, 2024 21:29
Copy link

github-actions bot commented May 9, 2024

Benchmark for 54725e9

Click to view benchmark
Test Base PR %
code_action 5.5±0.12ms 5.4±0.09ms -1.82%
code_lens 291.3±8.50ns 289.2±8.52ns -0.72%
compile 6.8±0.13s 4.0±0.03s -41.18%
completion 5.0±0.15ms 4.7±0.09ms -6.00%
did_change_with_caching 6.2±0.08s 3.5±0.06s -43.55%
document_symbol 954.8±24.33µs 1002.8±46.17µs +5.03%
format 87.6±1.43ms 89.8±0.53ms +2.51%
goto_definition 352.6±5.98µs 373.4±6.17µs +5.90%
highlight 9.2±0.19ms 9.0±0.02ms -2.17%
hover 586.2±7.11µs 496.0±6.17µs -15.39%
idents_at_position 119.3±0.46µs 123.9±1.01µs +3.86%
inlay_hints 642.2±19.20µs 672.2±17.26µs +4.67%
on_enter 495.9±13.86ns 485.5±19.95ns -2.10%
parent_decl_at_position 3.7±0.03ms 3.7±0.04ms 0.00%
prepare_rename 348.2±7.76µs 365.1±5.53µs +4.85%
rename 9.7±0.13ms 9.6±0.12ms -1.03%
semantic_tokens 1048.4±26.45µs 1046.4±19.65µs -0.19%
token_at_position 345.6±2.28µs 368.5±2.41µs +6.63%
tokens_at_position 3.7±0.13ms 3.8±0.14ms +2.70%
tokens_for_file 433.4±3.71µs 418.6±2.04µs -3.41%
traverse 51.2±1.32ms 40.9±1.07ms -20.12%

JoshuaBatty added a commit that referenced this pull request May 10, 2024
…5983)

## Description
Using the same performance test as in #5976 and #5978, this change gives
a further 21.33% performance improvement.

Before: Elapsed time: `14.653166459s`
After: Elapsed time: `11.527667375s`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
compiler General compiler. Should eventually become more specific as the issue is triaged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants