-
Notifications
You must be signed in to change notification settings - Fork 323
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
Avoid copying IR nodes when no change is performed in passes #10839
Conversation
This PR ensures that we only perform IR copying when actually needed, i.e. when one of the fields has changed. Most of the changes should not be controversial except for equality change in MetadataStorage. Without it, an empty pass metadata would be treated as equal, therefore preventing duplication from happening.
There is a runtime-parser test failure:
as I discovered later this is probably due to difference of |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Magnificent work on the copy
methods rewrite. They should have been generated... if we ever get to rewriting this to Java, please use annotation processor to generate these copy
& co. methods.
...untime-compiler/src/main/java/org/enso/compiler/pass/analyse/PrivateConstructorAnalysis.java
Show resolved
Hide resolved
...me-integration-tests/src/test/scala/org/enso/compiler/test/core/ir/MetadataStorageTest.scala
Show resolved
Hide resolved
Avoid expensive computations for static methods.
I can't get flamegraphs similar to #9236 (comment) working but I've noticed in our profiling that we do much less copying now. At the expense of more equality checks, so you give some and you take some. |
Pull Request Description
This PR attempts to avoid creating unnecessary IR nodes. Whenever none of the components of any IR node is different from the original one, there is no need to create a new instance of it.
Renders #10718 obsolete.
Important Notes
I spent quite a lot of time debugging problems with this approach (compared to #10718) only to find out that the problem was in the overriden definition of
MetadataStorage.equals
, that prevented duplication from working correctly.Checklist
Please ensure that the following checklist has been satisfied before submitting the PR:
Scala,
Java,
TypeScript,
and
Rust
style guides. In case you are using a language not listed above, follow the Rust style guide.