-
Notifications
You must be signed in to change notification settings - Fork 1.1k
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
Space: Cache isSubspace for a 3000% speedup #13637
Conversation
Who's interested and/or good at performance? I've seen commits and hears @odersky speak about doing some. Also @liufengyun might have some feedback specific to the space engine in general. Review feedback welcome. |
It's nice to improve performance 👍 I'd suggest adding the test to benchmarks, and also run a bench test to see how it helps existing bench tests related to exhaustivity. |
I've not looked at the benchmarks? What am I adding and how? And is that instead of the existing test or in addtion?
Do I need to have write access to the staging repo to create a branch there to do that still? And what's the next step after that? Sorry... this would be my first time playing the benchmarking game in the dotty repo. |
You can add a new exhaustivity check benchmark in the following file: https://github.com/lampepfl/dotty/blob/master/bench/profiles/exhaustivity.yml And then tell the bot to run the bench test with a comment like I'm not sure if you need to push to staging repo (you can first try without doing that). You will definitely need to add yourself to be known to the bot here: https://github.com/lampepfl/bench/blob/master/bin/process#L22 |
d6dac0f
to
c6ee5e3
Compare
test performance with #exhaustivity please |
It seems the Bot is not running, the same in #13715 |
test performance with #exhaustivity please |
performance test scheduled: 1 job(s) in queue, 1 running. |
Performance test finished successfully: Visit https://dotty-bench.epfl.ch/13637/ to see the changes. Benchmarks is based on merging with master (1ed25ce) |
The numbers look nice 👍 Regarding the implementation, I'd suggest trying caching the subtyping --- if the numbers are similar, caching for subtyping is better. |
Caching the subtyping seems to be much worst:
Happy to approve this? |
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.
LGTM 👍
No description provided.