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

Optimize AliasAnalysis hotspot #8701

Merged
merged 2 commits into from
Jan 11, 2024

Conversation

hubertp
Copy link
Contributor

@hubertp hubertp commented Jan 8, 2024

Pull Request Description

scopeFor is a real hotspot that hasn't been particularly optimized. By using iterator and a simple variable instead of a list and checking its length I was able to get 10% speedup.
Note that it seems tempting to throw the exception within the loop but that seems to create a less optimized code.

Important Notes

I'm consistently getting ~10% speedup on a hello world example (with standard libraries).

For example I'm no longer seeing 20ms spent in scopeFor and most of them are below 10ms:
Screenshot from 2024-01-08 12-15-30

Before
Screenshot from 2024-01-08 12-17-32

After
Screenshot from 2024-01-08 12-17-09

Checklist

Please ensure that the following checklist has been satisfied before submitting the PR:

  • Screenshots/screencasts have been attached, if there are any visual changes. For interactive or animated visual changes, a screencast is preferred.
  • All code follows the
    Scala,
    Java,
    and
    Rust
    style guides. In case you are using a language not listed above, follow the Rust style guide.

@hubertp hubertp added the CI: No changelog needed Do not require a changelog entry for this PR. label Jan 8, 2024
Copy link
Member

@JaroslavTulach JaroslavTulach left a comment

Choose a reason for hiding this comment

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

Nice finding!

From a strategic perspective we should first address

and only then integrate this bugfix. Then we could demonstrate on the graphs how much we do care about static compiler speed.

On the other hand, we do have the Startup_empty_startup benchmark and if that one is improving, then we have an (at least indirect) evidence as well.

Copy link
Member

@Akirathan Akirathan left a comment

Choose a reason for hiding this comment

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

Very nice improvement. Just out of curiosity: Do you have profiling snapshots? If so, please attach some screenshots from them.

@hubertp
Copy link
Contributor Author

hubertp commented Jan 8, 2024

Yes, I agree we need proper benchmarks. As I talked to @4e6 I think there is a big scope for improving the compiler performance, we haven't even started.

I was hoping that Startup benchmark will confirm my numbers. Started it.

I also added some screenshots. It's a bit hard to show but basically traversing a list is gone.

@hubertp
Copy link
Contributor Author

hubertp commented Jan 8, 2024

Screenshot from 2024-01-08 15-28-57

hmm, that's weird. it looks like no difference? I'm clearly getting better results locally 🤔

@hubertp
Copy link
Contributor Author

hubertp commented Jan 8, 2024

Maybe the reason is because it is not built against latest develop. I will retry.

@hubertp hubertp force-pushed the wip/hubert/optimize-alias-analysis-hotspot branch from ef26afc to d268be8 Compare January 8, 2024 14:39
`scopeFor` is a real hotspot that hasn't been particularly optimized.
By using iterator and a simple variable instead of a list and checking
its length I was able to get 10% speedup.
Note that it seems tempting to throw the exception within the loop but
that seems to create a less optimized code.
@hubertp hubertp force-pushed the wip/hubert/optimize-alias-analysis-hotspot branch from d268be8 to 3eed80f Compare January 8, 2024 22:49
@hubertp
Copy link
Contributor Author

hubertp commented Jan 9, 2024

Screenshot from 2024-01-09 10-15-09
I think something is wrong with the benchmark because I'm clearly getting improvements locally. Not sure what to do with that one for the time being.

@hubertp
Copy link
Contributor Author

hubertp commented Jan 9, 2024

Here are files with VisualVM samping
before and after

@hubertp hubertp added the CI: Keep up to date Automatically update this PR to the latest develop. label Jan 10, 2024
@hubertp hubertp added CI: Ready to merge This PR is eligible for automatic merge and removed CI: Keep up to date Automatically update this PR to the latest develop. labels Jan 10, 2024
@mergify mergify bot merged commit a94cad6 into develop Jan 11, 2024
36 of 37 checks passed
@mergify mergify bot deleted the wip/hubert/optimize-alias-analysis-hotspot branch January 11, 2024 15:49
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
CI: No changelog needed Do not require a changelog entry for this PR. CI: Ready to merge This PR is eligible for automatic merge
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants