[PlutusTx] Optimize the 'Eq' instance of 'Value' #5593
Conversation
This reverts commit 854cb90.
…to effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
| 13789 |
There was a problem hiding this comment.
Much better than last time.
| ({cpu: 310936611 | ||
| | mem: 1245566}) |
There was a problem hiding this comment.
A bit worse than last time (it was 243063611, but with a buggy version, so who knows if it even was correct), but still good.
There was a problem hiding this comment.
Relocated some definitions from a test stanza, feel free to ignore this file, everything new here has already been reviewed.
| ({cpu: 3012083773 | mem: 11175942}) | ||
| ({cpu: 713195980 | mem: 3098404}) |
There was a problem hiding this comment.
Note that those tests were written with the assumption that duplicated CurrencySymbols/TokenNames were allowed, hence they're pretty useless for an algo that quits early in certain cases when we have duplicated names. Fixing that is a future work unrelated to this PR.
| ({cpu: 6164100 | mem: 26900}) | ||
| ({cpu: 8715491 | mem: 36702}) | ||
| ({cpu: 10430980 | mem: 42904}) |
There was a problem hiding this comment.
These tests are a bit better, but still suffer from the same problem. But even then, at least some of the numbers we see here are representative of the actual cost changes.
| -- "cbade" | ||
| appendR :: forall a. [a] -> [a] -> [a] | ||
| appendR = rev where | ||
| rev :: [a] -> [a] -> [a] |
There was a problem hiding this comment.
Just replicating the old logic to keep the diff minimal. We could've recursed in appendR directly, but that would be more costly due to that type-level a argument becoming a bunch of delay and force on the UPLC side (which is really annoying).
| ({cpu: 552055724 | ||
| | mem: 1974822}) |
There was a problem hiding this comment.
Some of these are pretty decent.
| ({cpu: 859760620 | ||
| | mem: 2993308}) |
There was a problem hiding this comment.
Another example.
| We check equality of values of two key-value pairs right after ensuring that the keys match. This is | ||
| disadvantageous if the values are big and there's a key that is present in one of the lists but not | ||
| in the other, since in that case computing equality of values was expensive and pointless. However |
There was a problem hiding this comment.
Different to the previous PR, 'cause I changed my mind.
effectfully
force-pushed
the
effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
branch
from
943bd22 to
468522b
Compare
…to effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
…to effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
effectfully
force-pushed
the
effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
branch
from
45817e8 to
a4838d6
Compare
|
You might have some difficulty merging this now: see #5587. |
…to effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
Somewhat surprisingly, it wasn't troubling at all. |
There was a problem hiding this comment.
This would be a lot easier to review if we had all of the test outputs in one file ...
There was a problem hiding this comment.
Also, what happened to the role-payout outputs? No change?
There was a problem hiding this comment.
No change?
I suppose. I have no idea really what to expect w.r.t. changes to Marlowe benchmarks.
There was a problem hiding this comment.
I checked the code and the role-payout script (it's quite simple) doesn't use == on Values. The semantics one does though (here).
| -- equal pointwise) with a bit of additional logic to get some easy performance gains. | ||
| | kL == kR0 = if vL `eqV` vR0 then goBoth kvsL' kvsR0' else False | ||
| | is0 vL = goBoth kvsL' kvsR0 | ||
| | otherwise = goRight [kvR0 | not $ is0 vR0] kvsR0' |
There was a problem hiding this comment.
The list comprehension is a slightly unexpected way of writing this. Does it compile any differently from an if-then-else version?
There was a problem hiding this comment.
I don't think it's any different, I'm just very used to writing zero-or-one-elements-depending-on-a-condition lists like that. I can change it if you find it confusing, although there are many places where I use that idiom. Do feel free to request not to use the idiom.
There was a problem hiding this comment.
No that's fine, just leave it. I'd never seen this before, but the problem that it's solving is something that I've come across many times.
| -> Property | ||
| onLists value f = forAll (fmap listsToValue . f $ valueToLists value) | ||
|
|
||
| -- | Test that the 'Monoid' instance of 'Value' is law-abiding and that merging two non-zero values |
There was a problem hiding this comment.
It's not just testing that it's a monoid, but that it's a commutative monoid (just nitpicking).
There was a problem hiding this comment.
I'd forgotten this, but I now realise that it is in fact a group, since every element has an inverse.
| arbitrary = do | ||
| -- Generate values for all of the 'TokenName's in the final 'Value' and split them into a | ||
| -- list of lists. | ||
| faceValues <- fmap (map getNonEmpty) . multiSplit . map unFaceValue =<< arbitrary |
There was a problem hiding this comment.
This is generating Values where no CurrencySymbol has an empty list of tokens, is that correct? Do we actually want to exclude such things? I can create them manually and they seem to behave as you'd expect (for example, applying split to such a thing gives me back two Values with a currency symbol with an empty list of tokens).
I was wondering if this situation was prohibited by the ledger, but presumably it isn't since enforcing it would require quite a lot of checking. Now I'm worrying about whether it effects equality. Do we consider a value with a currency symbol which has no tokens to be equal to the same thing with that currency symbol missing? The version of equality implemented in this PR doesn't consider them to be equal. We should check whether equality of values is specified in the ledger specification or somewhere.
There was a problem hiding this comment.
I originally thought that the version of == implemented here didn't consider currency symbols with no tokens to be the same as absent currency symbols, but it does (and it says so here, which is a relief). However, I still think that this generator isn't generating such things and it probably should be.
There was a problem hiding this comment.
This is generating Values where no CurrencySymbol has an empty list of tokens, is that correct?
Yes, and thank you for spotting that, it's really a generation bug. I've now fixed it, please review the commit. I'll post some example distributions tomorrow or so.
I originally thought that the version of == implemented here didn't consider currency symbols with no tokens to be the same as absent currency symbols, but it does (and it says so here, which is a relief).
Yep, I didn't need to change anything in the implementation once I added generation of empty lists, tests kept being green (hope CI doesn't disprove that).
|
Having looked at the tests again, it might be worth testing that |
…to effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
| -- more likely to hit a corner case related to handling elements at the beginning or the end of a | ||
| -- list). | ||
| multiSplit :: Double -> [a] -> Gen [[a]] | ||
| multiSplit endProb = multiSplit1 >=> insertManyPreferEnds endProb [] . coerce |
There was a problem hiding this comment.
The tests are more complicated than the code they're testing now! I'm reasonably convinced (mostly by looking at the output of the generators) that they're doing the right things though.
There was a problem hiding this comment.
Oh, I forgot to report here. I did look at the output of the generators closely for much larger probability of zeroes and more test runs and it was quite great, here's the output for a one-element list (newlines added for readability):
+++ OK, passed 100000 tests:
12.967% [1]
9.641% [0,1]
9.614% [1,0]
6.346% [1,0,0]
6.341% [0,0,1]
6.305% [0,1,0]
4.087% [0,1,0,0]
3.898% [1,0,0,0]
3.875% [0,0,1,0]
3.850% [0,0,0,1]
2.367% [0,1,0,0,0]
2.344% [1,0,0,0,0]
2.336% [0,0,0,0,1]
2.308% [0,0,0,1,0]
2.293% [0,0,1,0,0]
1.384% [0,0,0,1,0,0]
1.377% [0,1,0,0,0,0]
1.360% [0,0,1,0,0,0]
1.314% [0,0,0,0,1,0]
1.295% [1,0,0,0,0,0]
1.271% [0,0,0,0,0,1]
0.809% [0,0,0,1,0,0,0]
0.759% [0,0,1,0,0,0,0]
0.758% [0,0,0,0,1,0,0]
0.753% [0,1,0,0,0,0,0]
0.745% [0,0,0,0,0,0,1]
0.720% [1,0,0,0,0,0,0]
0.718% [0,0,0,0,0,1,0]
<...>
and for a two-element list:
6.538% [1,1]
6.390% [2]
4.830% [0,2]
4.828% [2,0]
3.326% [0,1,1]
3.269% [0,2,0]
3.221% [1,1,0]
3.146% [1,0,1]
3.140% [0,0,2]
3.133% [2,0,0]
1.969% [2,0,0,0]
1.949% [0,2,0,0]
1.918% [0,0,2,0]
1.901% [0,0,0,2]
1.658% [1,1,0,0]
1.634% [0,1,1,0]
1.582% [0,0,1,1]
1.559% [0,1,0,1]
1.508% [1,0,1,0]
1.475% [1,0,0,1]
1.188% [0,0,2,0,0]
1.158% [2,0,0,0,0]
1.147% [0,0,0,0,2]
1.138% [0,2,0,0,0]
1.119% [0,0,0,2,0]
0.874% [0,0,0,1,1]
0.859% [1,1,0,0,0]
0.823% [0,0,1,0,1]
0.817% [0,0,1,1,0]
0.807% [0,1,1,0,0]
0.771% [1,0,1,0,0]
0.741% [0,1,0,0,1]
0.733% [0,1,0,1,0]
0.732% [1,0,0,1,0]
You can see that there's some skewness in for example
1.658% [1,1,0,0]
1.634% [0,1,1,0]
1.582% [0,0,1,1]
1.559% [0,1,0,1]
1.508% [1,0,1,0]
1.475% [1,0,0,1]
but that's by design, since I made it so that inserting a zero (or an empty list etc) in the middle of a list happens a bit less often than doing it at one of the ends, which is precisely what we're observing.
…to effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
effectfully
deleted the
effectfully/plutus-tx/experiment-with-MatchUnclear-and-tildas
branch
Attempt number 3. Now with proper tests finally. I checked that the tests find all bugs from the previous PR (and one more). I absolutely didn't overfit them for that purpose, I'd written something most general first and only then checked. I entertained the idea of manually sorting out all possible corner cases, then building tests based on that, but the most straightforward QuickCheck thing really does seem to have a more or less equal power than whatever I would've wasted at least a week on, so I decided not to bother.
The new algorithm for checking equality of
Values is both faster and simpler. It's practically a reimplementation ofvalueEqualsValue4from #5135 (it may or may not differ, I didn't attempt to formally compare the two), so kudos to @manupadillaph.