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

Memory usage reduction #6249

Merged
merged 4 commits into from
Jul 9, 2024
Merged

Memory usage reduction #6249

merged 4 commits into from
Jul 9, 2024

Conversation

jjcnn
Copy link
Contributor

@jjcnn jjcnn commented Jul 9, 2024

Description

#6044 increased the memory usage of the compiler by an estimated 400%. This PR alleviates some of that increased footprint.

is_external in the Module struct needs to be updated to allow for compilation of dependencies (a module is not external while it is being compiled, but is external when one of its dependents is being compiled). #6044 introduced an extra level of cloning, which in this PR is changed by making Module mutable and destructively updating is_external.

Checklist

  • I have linked to any relevant issues.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have updated the documentation where relevant (API docs, the reference, and the Sway book).
  • I have added tests that prove my fix is effective or that my feature works.
  • I have added (or requested a maintainer to add) the necessary Breaking* or New Feature labels where relevant.
  • I have done my best to ensure that my PR adheres to the Fuel Labs Code Review Standards.
  • I have requested a review from the relevant team or maintainers.

Copy link

github-actions bot commented Jul 9, 2024

Benchmark for f124653

Click to view benchmark
Test Base PR %
code_action 5.2±0.09ms 5.2±0.08ms 0.00%
code_lens 287.8±13.22ns 284.4±8.65ns -1.18%
compile 2.7±0.02s 2.6±0.04s -3.70%
completion 4.7±0.16ms 4.7±0.11ms 0.00%
did_change_with_caching 2.5±0.02s 2.6±0.03s +4.00%
document_symbol 878.0±25.47µs 862.7±32.21µs -1.74%
format 71.6±0.97ms 72.0±1.76ms +0.56%
goto_definition 344.3±8.81µs 342.4±9.84µs -0.55%
highlight 8.8±0.20ms 9.0±0.06ms +2.27%
hover 501.3±6.94µs 497.3±13.86µs -0.80%
idents_at_position 118.7±0.44µs 119.2±0.77µs +0.42%
inlay_hints 631.3±33.49µs 642.5±21.34µs +1.77%
on_enter 471.9±14.41ns 446.1±15.88ns -5.47%
parent_decl_at_position 3.6±0.03ms 3.7±0.03ms +2.78%
prepare_rename 342.3±7.29µs 342.1±5.64µs -0.06%
rename 9.2±0.18ms 9.4±0.08ms +2.17%
semantic_tokens 1231.7±11.59µs 1263.2±19.11µs +2.56%
token_at_position 335.8±2.01µs 335.6±2.52µs -0.06%
tokens_at_position 3.5±0.06ms 3.7±0.02ms +5.71%
tokens_for_file 395.7±1.82µs 400.2±5.68µs +1.14%
traverse 38.0±1.08ms 37.8±0.96ms -0.53%

@jjcnn jjcnn marked this pull request as ready for review July 9, 2024 14:55
@jjcnn jjcnn requested review from a team as code owners July 9, 2024 14:55
Copy link

github-actions bot commented Jul 9, 2024

Benchmark for 7c3dac0

Click to view benchmark
Test Base PR %
code_action 5.0±0.08ms 5.2±0.07ms +4.00%
code_lens 287.8±10.08ns 289.1±8.13ns +0.45%
compile 2.7±0.04s 2.7±0.04s 0.00%
completion 4.5±0.01ms 4.7±0.07ms +4.44%
did_change_with_caching 2.6±0.03s 2.5±0.04s -3.85%
document_symbol 887.8±14.05µs 889.0±53.50µs +0.14%
format 72.6±2.50ms 71.9±2.75ms -0.96%
goto_definition 346.8±7.88µs 343.6±8.55µs -0.92%
highlight 8.7±0.12ms 9.0±0.05ms +3.45%
hover 503.1±6.24µs 493.8±6.51µs -1.85%
idents_at_position 117.5±1.82µs 118.0±1.17µs +0.43%
inlay_hints 630.3±27.36µs 643.3±18.32µs +2.06%
on_enter 476.2±27.51ns 442.1±14.11ns -7.16%
parent_decl_at_position 3.5±0.04ms 3.7±0.04ms +5.71%
prepare_rename 342.2±7.55µs 338.6±6.31µs -1.05%
rename 9.0±0.13ms 9.3±0.11ms +3.33%
semantic_tokens 1219.5±16.08µs 1255.3±15.59µs +2.94%
token_at_position 334.5±2.57µs 336.0±2.12µs +0.45%
tokens_at_position 3.5±0.03ms 3.7±0.05ms +5.71%
tokens_for_file 405.4±16.07µs 401.4±3.01µs -0.99%
traverse 38.5±0.53ms 37.8±0.98ms -1.82%

Copy link
Member

@kayagokalp kayagokalp left a comment

Choose a reason for hiding this comment

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

tooling changes ✅

@kayagokalp
Copy link
Member

This brought #6219 mem usage to ~1.7gb from ~20gb

@IGI-111 IGI-111 merged commit 5456e62 into master Jul 9, 2024
39 checks passed
@IGI-111 IGI-111 deleted the jjcnn/memory-usage-reduction branch July 9, 2024 22:16
@JoshuaBatty JoshuaBatty added the performance Everything related to performance, speed wise or memory wise. label Jul 10, 2024
esdrubal pushed a commit that referenced this pull request Aug 13, 2024
## Description

#6044 increased the memory usage of the compiler by an estimated 400%.
This PR alleviates some of that increased footprint.

`is_external` in the `Module` struct needs to be updated to allow for
compilation of dependencies (a module is not external while it is being
compiled, but is external when one of its dependents is being compiled).
#6044 introduced an extra level of cloning, which in this PR is changed
by making `Module` mutable and destructively updating `is_external`.

## Checklist

- [x] I have linked to any relevant issues.
- [x] I have commented my code, particularly in hard-to-understand
areas.
- [x] I have updated the documentation where relevant (API docs, the
reference, and the Sway book).
- [ ] If my change requires substantial documentation changes, I have
[requested support from the DevRel
team](https://github.com/FuelLabs/devrel-requests/issues/new/choose)
- [x] I have added tests that prove my fix is effective or that my
feature works.
- [x] I have added (or requested a maintainer to add) the necessary
`Breaking*` or `New Feature` labels where relevant.
- [x] I have done my best to ensure that my PR adheres to [the Fuel Labs
Code Review
Standards](https://github.com/FuelLabs/rfcs/blob/master/text/code-standards/external-contributors.md).
- [x] I have requested a review from the relevant team or maintainers.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
performance Everything related to performance, speed wise or memory wise.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants