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

Plutus Tx: avoid duplicating code for DEFAULT case bodies #5484

Merged
merged 4 commits into from
Aug 22, 2023

Conversation

michaelpj
Copy link
Contributor

Turns out we were duplicating code for a slightly subtle reason. GHC can have just a single DEFAULT case that handles a ll the non-explicitly-handled cases. But we need to make all the cases explicit, which means that we were duplicating the DEFAULT case for all the non-handled cases. And of course, the moment we see "duplicating code" it's a short hop to "exponential code", and indeed that's what I was seeing for code that has many nested default cases (which it turns out you get from using pattern synonyms, apparently).

The fix is simple: just share the code.

@michaelpj michaelpj requested a review from bezirg August 21, 2023 14:45
@michaelpj michaelpj force-pushed the mpj/exponential-default-bodies branch from 0e1681f to 5413409 Compare August 21, 2023 14:50
@michaelpj michaelpj force-pushed the mpj/exponential-default-bodies branch from 5413409 to b6d28a5 Compare August 21, 2023 15:01
Copy link
Member

@zliu41 zliu41 left a comment

Choose a reason for hiding this comment

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

There's a dramatic decrease in formula1 (size) but a 7.8% increase in formulaBudget. Can you look into it?

@michaelpj
Copy link
Contributor Author

I took a look at the PIR diff. It's not at all obvious what's worse: the changes are exactly what we'd expect (there was a lot of duplicated code from this, in fact). My guess is that

  • The formula PIR has a lot of default cases
  • The default cases are dynamically dead, i.e. we don't actually hit them
  • Since there are many default cases, the inliner doesn't inline the default body binding
  • So we now pay extra for the binding and don't benefit

The inlining decisions seem correct to me, since we would pay for it with a substantial increase in program size (as we can see). So I think we basically just get the opportunity to do a cost/size tradeoff differently, and we choose to not increase size, which seems fine.

@michaelpj michaelpj merged commit 967ae96 into master Aug 22, 2023
7 checks passed
@michaelpj michaelpj deleted the mpj/exponential-default-bodies branch August 22, 2023 09:31
D -> defaultBody
```
Then the inliner can inline it as appropriate.
-}
Copy link
Contributor

Choose a reason for hiding this comment

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

Great note

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.

3 participants