-
Notifications
You must be signed in to change notification settings - Fork 479
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
[Builtins] Make unlifting lazy again #6434
base: master
Are you sure you want to change the base?
[Builtins] Make unlifting lazy again #6434
Conversation
/benchmark validation |
/benchmark nofib |
/benchmark lists |
Click here to check the status of your benchmark. |
Comparing benchmark results of 'validation' on '4b8e137e1' (base) and 'd63521331' (PR) Results table
|
Click here to check the status of your benchmark. |
-2.3% in total. I don't trust it, probably just a glitch, let's see the next one. |
Comparing benchmark results of 'nofib' on '4b8e137e1' (base) and 'd63521331' (PR) Results table
|
Click here to check the status of your benchmark. |
-1.6% in total and I still don't believe it. Also note how the |
Comparing benchmark results of 'lists' on '4b8e137e1' (base) and 'd63521331' (PR) Results table
|
/benchmark lists |
2 similar comments
/benchmark lists |
/benchmark lists |
Click here to check the status of your benchmark. |
Comparing benchmark results of 'lists' on '4b8e137e1' (base) and 'd63521331' (PR) Results table
|
Click here to check the status of your benchmark. |
Comparing benchmark results of 'lists' on '4b8e137e1' (base) and 'd63521331' (PR) Results table
|
Click here to check the status of your benchmark. |
Comparing benchmark results of 'lists' on '4b8e137e1' (base) and 'd63521331' (PR) Results table
|
/benchmark validation |
/benchmark lists |
Click here to check the status of your benchmark. |
Click here to check the status of your benchmark. |
…to effectfully/builtins/make-unlifting-lazy-again
/benchmark validation |
Click here to check the status of your benchmark. |
Comparing benchmark results of 'validation' on 'ce72537391' (base) and '6478b97e0e' (PR) Results table
|
/benchmark validation |
2 similar comments
/benchmark validation |
/benchmark validation |
Click here to check the status of your benchmark. |
Comparing benchmark results of 'validation' on 'ce72537391' (base) and '6478b97e0e' (PR) Results table
|
Click here to check the status of your benchmark. |
Comparing benchmark results of 'validation' on 'ce72537391' (base) and '6478b97e0e' (PR) Results table
|
Click here to check the status of your benchmark. |
/benchmark validation |
1 similar comment
/benchmark validation |
Comparing benchmark results of 'validation' on 'ce72537391' (base) and '6478b97e0e' (PR) Results table
|
Click here to check the status of your benchmark. |
Comparing benchmark results of 'validation' on 'ce72537391' (base) and '6478b97e0e' (PR) Results table
|
Click here to check the status of your benchmark. |
Comparing benchmark results of 'validation' on 'ce72537391' (base) and '6478b97e0e' (PR) Results table
|
…to effectfully/builtins/make-unlifting-lazy-again
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.
This seems perfectly reasonable. It doesn't sound like it's going to impose a massive penalty on typical programs and it's probably better to avoid any possible surprises.
@@ -294,13 +294,29 @@ instance | |||
-- See Note [One-shotting runtime denotations]. | |||
-- Grow the builtin application within the received action and recurse on the result. | |||
toMonoF getBoth = BuiltinExpectArgument . oneShot $ \arg -> | |||
-- Ironically computing the unlifted value strictly is the best way of doing deferred |
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.
Ironically computing
This sounds like some kind of technique: "ironic computation". We should invent something just so we can call it that.
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.
Love the idea.
Co-authored-by: Kenneth MacKenzie <kenneth.mackenzie@iohk.io>
This is technically a regression performance-wise, but we may already be charging for it (depends on how budgeting calibration works), so let's see, maybe it's not detectable regardless.