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

Improve inferability of lineaxis and axis #1968

Merged
merged 10 commits into from May 30, 2022

Conversation

SimonDanisch
Copy link
Member

@SimonDanisch SimonDanisch commented May 23, 2022

@SimonDanisch SimonDanisch changed the title max methods inferability Use max_methods and improve inferability May 23, 2022
@MakieBot
Copy link
Collaborator

MakieBot commented May 23, 2022

Compile Times benchmark

Note, that these numbers may fluctuate on the CI servers, so take them with a grain of salt.

using time

master:  9.37 < 9.50 > 9.64, 0.09+-
pr:      9.35 < 9.41 > 9.56, 0.08+-
speedup: 1.00 < 1.00 > 1.01, 0.01+-
median:  -0.96% => invariant

This PR does not change the using time.

ttfp time

master   27.05 < 27.33 > 27.96, 0.38+-
pr       26.14 < 26.31 > 26.64, 0.21+-
speedup: 1.02 < 1.04 > 1.07, 0.01+-
median:  -3.73% => invariant

This PR does not change the ttfp time.

@SimonDanisch
Copy link
Member Author

@IanButterworth, you may want to check this PR out, as I turned quite a lot of observables into "change observables" while at it...
I don't have a benchmark setup right now, so not 100% sure if I hit the right observables and how much it makes a difference.

@ffreyer
Copy link
Collaborator

ffreyer commented May 24, 2022

Does max_methods just shift compile time to run time? It kinda seems that way from the benchmarks

@SimonDanisch
Copy link
Member Author

It kinda seems that way from the benchmarks

What benchmarks? We don't have run time benchmarks and both using + ttfp times are an improvement

Does max_methods just shift compile time to run time

Depends on the code, fully typed + inferable code shouldn't change. Only code which calls functions, where the arguments aren't fully inferable may get slower runtime dispatch.

@ffreyer
Copy link
Collaborator

ffreyer commented May 24, 2022

Iirc the first compile time benchmark I saw here had around -15% on using and maybe 1% more or less on ttfp. With all the other changes now I don't know what's actually improving things. If the ttfp improvements are mostly from change observables it might not be worth having the experimental stuff. Especially since "experimental" hints that it may break soon.

@SimonDanisch
Copy link
Member Author

specially since "experimental" hints that it may break soon.

It was regarded as a bad move to name the module Experimental, because in hindsight, it wasn't really meant like that... So I think we can safely us it for now.

Some more detailed benchmarks:

branch using ttfp runtime
this 4.8 14 19.8, 3.5mb
this, no max_method 5.6 14 19.8ms, 3.5mb
master 5.7 15.2 21ms, 3.68 MiB

I think this shows nicely, that max_methods gives us significant less using times, without influencing ttfp or runtime, while the net improvement of the other changes are pretty nice on top.

@ffreyer
Copy link
Collaborator

ffreyer commented May 25, 2022

Oh I thought ttfp would include using time as well. So then this is really just a straight up improvement

@SimonDanisch SimonDanisch changed the title Use max_methods and improve inferability Improve inferability of lineaxis and axis May 30, 2022
@SimonDanisch SimonDanisch merged commit 6b851b5 into master May 30, 2022
@SimonDanisch SimonDanisch deleted the sd/max-methods-inferability branch May 30, 2022 13:46
@SimonDanisch SimonDanisch mentioned this pull request Jun 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants