Fixes Numeric type propagation #6461
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
esdrubal
added
bug
Benchmark for 9178377Click to view benchmark
|
Benchmark for cd213fdClick to view benchmark
|
Benchmark for d742809Click to view benchmark
|
Benchmark for 26a45ebClick to view benchmark
|
Benchmark for 6e2b942Click to view benchmark
|
Benchmark for 943be69Click to view benchmark
|
To fix the Numeric type propagation we now do the type checking of code blocks twice, first to collect all the unification and second to unify the types of a previous namespace with the variable declarations types of the current namespace. This changes incurs a significative performance drop in the compilation time. The test in release goes from 3 secs to 5 secs. This can be improved significatively when we start using LexicalScopes instead of cloning the whole namespace on each scope. Fixes #6371
esdrubal
force-pushed
the
esdrubal/6371
branch
from
934d19e to
52b34c5
Compare
Benchmark for b5cd011Click to view benchmark
|
Benchmark for d3b931eClick to view benchmark
|
IGI-111
previously approved these changes
Aug 26, 2024
|
25% performance regression is pretty hard pill to swallow. Is there an open issue for removing the namespace cloning and using LexicalScopes like you mention @esdrubal ? If not, would you mind opening one and linking this PR to the issue so this regression can at least just be temporary? |
esdrubal
force-pushed
the
esdrubal/6371
branch
from
880cc3b to
80e1d2d
Compare
esdrubal
force-pushed
the
esdrubal/6371
branch
from
87b1caf to
99d1f5b
Compare
IGI-111
previously approved these changes
Aug 27, 2024
Benchmark for c505e0aClick to view benchmark
|
tritao
previously approved these changes
Aug 27, 2024
tritao
reviewed
Aug 27, 2024
Benchmark for cdd3fdbClick to view benchmark
|
|
This PR is also hitting the recursion limit that #6471 will fix. |
IGI-111
approved these changes
Aug 28, 2024
tritao
approved these changes
Aug 28, 2024
Benchmark for 4b33516Click to view benchmark
|
8 tasks
JoshuaBatty
added a commit
that referenced
this pull request
Aug 28, 2024
## Description As shown in https://fuellabs.github.io/sway-performance-dashboard/ #6461 introduced a bigger memory footprint. Lexical scope has a shared OrdMap called `symbols_unique_while_collecting_unifications` that was never cleared. With this commit, it is cleared on each code block along with the clear of collected unifications. ## Checklist - [x] I have linked to any relevant issues. - [x] 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). - [ ] If my change requires substantial documentation changes, I have [requested support from the DevRel team](https://github.com/FuelLabs/devrel-requests/issues/new/choose) - [ ] 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. Co-authored-by: João Matos <joao@tritao.eu> Co-authored-by: Joshua Batty <joshpbatty@gmail.com>
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
To fix the Numeric type propagation we now do the type checking of code blocks twice, first to collect all the unification and second to unify the types of a previous namespace with the variable declarations types of the current namespace.
This change incurs a performance drop in the compilation time. The benchmark
compiletakes 25% more time to run.This can be improved significantly by using LexicalScopes instead of cloning the whole namespace on each scope.
Fixes #6371
Checklist
Breaking*orNew Featurelabels where relevant.