[Builtin] Quit using 'SomeConstantOf' #4173
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
@kwxm could you please run the built-in lists benchmarks on this PR? |
effectfully
changed the title
Will do. Shea's currently trying to get the automatic benchmarking to work again, so that's occupying the machine a lot. I'll run the benchmarks when it's quieter. |
|
Thank you! |
|
As requested. Looks promising. |
|
Hmmm. I ran the validation benchmarks just to see what happened and got this: I was under the impression that calling builtins only accounted for about 15% of validation time, but I'm going to have to think again. Some of the nofib benchmarks call builtins a lot, so I'll run them tomorrow and see what happens. |
|
There's no apparent difference in the nofib examples: everything's pretty much within ±1%. |
|
Actually that's not quite true. Some programs are consistently a little faster, some are consistently a little slower. The differences are very small though. |
|
Huh, that's a really surprisingly large difference! I'm quite surprised... |
|
Certainly not going to complain, though. |
michaelpj
approved these changes
Nov 2, 2021
| @@ -658,6 +659,28 @@ instance (HasConstantIn uni term, KnownTypeAst uni rep) => | |||
| makeKnown _ = pure . fromConstant . unSomeConstant | |||
| readKnown mayCause = fmap SomeConstant . asConstant mayCause | |||
|
|
|||
| newtype SomeConstantPoly uni f reps = SomeConstantPoly | |||
There was a problem hiding this comment.
Not ready for review!
Yep, this should be the case as this PR only touches functions over polymorphic built-in types and I guess we have none in
Hm, weird then.
I was thinking about moving it to the I guess this PR can be merged as is though, once I write some docs and move |
Aha, that makes me guess what this is: it's probably speeding up all the decoding of |
I think we should definitely merge it, given the speedup, happy to wait until you make it nicer as you like. |
|
I ran the validation benchmarks again with a freshly-pulled master to make sure that I hadn't done anything silly. SImilar results. |
|
But wait! Everything's slower today. Here's what happens if I compare the results for yesterday and today. I have no idea what's going on. According to |
|
Well, as long as the diff from with and without this PR is the same today as yesterday, I'm not too bothered. |
effectfully
force-pushed
the
effectfully/builtins/ignore-SomeConstantOf
branch
from
86df0b2 to
9452629
Compare
|
Ok, I've moved a few definitions around, added a bit of docs and didn't touch anything else, so given @michaelpj's approval, I'm gonna merge this. I'll try unifying |
MaximilianAlgehed
pushed a commit
to Quviq/plutus
that referenced
this pull request
Mar 3, 2022
It was pointless and inefficient when there's no universe polymorphic. Now we have `SomeConstantPoly`, which serves the same purpose, but is faster, even though it's not as safe.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
This drops all occurrences of
SomeConstantOffrom the definitions of builtins. Since we hardcodeDefaultUnithere anyway, we can just match on tags directly, which really should be at least somewhat more performant (if it's not, then I'm optimizing something that is not even close to being a bottleneck).Not ready for review, just for benchmarking.