Optimize compilation speed and memory consumption (DeclEngine)#7659
Conversation
PR SummaryHigh Risk Overview
Reviewed by Cursor Bugbot for commit 1bda649. Bugbot is set up for automated code reviews on this repo. Configure here. |
Merging this PR will improve performance by 19.86%
Performance Changes
Tip Curious why this is faster? Comment Comparing |
|
👍 |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit 1bda649. Configure here.
JoshuaBatty
left a comment
There was a problem hiding this comment.
💪
Nice finds and fixes. Love it. Well done!
…7662) ## Description This PR is a continuation of #7659 and implements first two points of the proposed Next Steps by: - moving `HasChanges` to a separate `has_changes` module, - returning `HasChanges` from `ReplaceDecls`, instead of `bool`, - returning `HasChanges` from `UpdateConstantExpression`, instead of `()`, - returning `HasChanges` from `MaterializeConstGenerics`, instead of `()`. This PR has a focus on refactoring and not on utilizing the information provided by returned `HasChanges`. Therefore, some implementations could be further simplified but are deliberately left as-is to be as close to straightforward refactoring as possible. E.g., `materialize_const_generics` implementations, like e.g., for `TypeId` can be simplified. Simpler implementation that also check the return value and potentially remove additional unnecessary inserts into `DeclEngine` will be done in a follow up PR. ## 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. - [ ] 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.

Description
This PR improves compiler's memory consumption and compilation speed by removing duplicated entries from the
DeclEngine. The PR also removes many of the unnecessary cloning of various type declarations.When compiling o2
order-bookcontract:Compilation time improvements are also reported by CodeSpeed:
BASEHEADcompileopen_all_example_workspace_membersWhen compiling the o2
order-bookcontract, the number of declarations on theDeclEnginereduced as following (only biggest three slabs shown):Approach
To efficiently develop large Sway applications like o2 we need to reduce compiler memory consumption and compilation time.
DeclEnginewas holding large number of duplicated declarations, redundantly inserted during different compiler phases, e.g., monomorphization. Implementing SemanticDefinitions RFC will remove all of those duplicates by changing how monomorphization work.Implementing
SemanticDefinitionswill require a long(er) roadmap. To get more performant compiler immediately this PR checks if declarations to be inserted are actually changed compared to originals that are already in theDeclEngine.Note that this practice was already used in some cases, where
HasChangeswas checked after callingsubst, but these cases were very rare.Side-effects
Assuming how intertwined type checking, type unification, type inference, and monomorphization is, I've expected more side-effect of removing the duplicates, but it turned out there were only two:
unused_return_valuetest) reported two additional warnings that are valid warnings and were not reported before.DeclEngineinsert-elimination inmonomorphize_methodbreaks recursion detection #7658. As described in that issue, recursion detection must not depend on duplicates being inserted. Essentially, the current implementation relies on an unwanted behavior. Because the probability of having recursive methods is rare, and there is no miscompilation (compiler overflows), this issue can be analyzed and fixed in a separate PR so that we can immediately have the benefits of not-inserting duplicates in themethod_application.Aside from these two changes in the compilation process, the compilation output (bytecode) remained the same.
Next Steps
Additional improvements can be done in subsequent PRs.
HasChangesenum is still defined insubst_types. It should be moved to some common place. [Code quality]HasChanges, returningbool, or not returning information at all. We want to haveHasChangeseverywhere as an established pattern. [Code quality]HasChangesshould be marked as#[must_use]with appropriate hint message. This will help to not forget to check the change and also to easy discover additional places where we, e.g., insert intoDeclEnginealthough there are no changes. [Code quality, Performance]DeclEngine. A part of the reason is that we still insert same monomorphized types if they are created by monomorphizing the same parent with the same generic arguments but in different places in code. We can explore approaches to remove these duplicates as well. E.g, have hashing/cashing inside of theDeclEngineto avoid duplicates, while avoiding at the same time hashing cost of large decls likeTyFunctionDecl? Isn't that whatTyFunctionSignatureis doing? [Performance]Checklist
Breaking*orNew Featurelabels where relevant.