[Plinth] Optimize 'unionValue' #6590
Conversation
effectfully
temporarily deployed
to
github-pages
GitHub Actions
Inactive
| That b -> f 0 b | ||
| These a b -> f a b | ||
| in Value (fmap (fmap unThese) combined) | ||
| unionWith f = coerce (Map.unionWith @CurrencySymbol (Map.unionWith @TokenName f)) |
There was a problem hiding this comment.
@zliu41 I don't really know much about Value stuff, is this actually correct?
There was a problem hiding this comment.
I'm not sure that's right. In the original, whenever there isn't a collision, the input function f is applied with the respective value and a 0. I don't see that happening in this new version, unless I'm reading the code wrong.
There was a problem hiding this comment.
Yes, semantics is different. But isn't it correct now? It was quite weird before, I'd argue.
There was a problem hiding this comment.
Good point. It should be documented that it treats [] and [(token, 0)] as different Values (so does the old implementation, but in a slightly different way).
There was a problem hiding this comment.
So what is the expected behaviour of unionWith? What does it mean that this version is correct?
There was a problem hiding this comment.
Well, I by default expect the standard behavior, i.e. the one in containers.
On the other hand, maybe I'm wrong, maybe the old behavior of unionValue (-) is the correct one. But in that case we should introduce
unionWithDef :: forall k a. Eq k => a -> (a -> a -> a) -> Map k a -> Map k a -> Map k awhich feed the first argument into the second whenever a key is missing in one of the maps.
So yeah, I think you've convinced me that the old behavior was correct and my new one isn't. @zliu41 opinion?
There was a problem hiding this comment.
The old behavior doesn't treat a token with zero quantity and a token that doesn't exist the same way either. e.g.,
unionWith (\_ _ -> 42) [] [] == []
unionWith (\_ _ -> 42) [(token, 0)], [] == [(token, 42)]
This doesn't look any more correct than the new behavior.
So what is the expected behaviour of unionWith? What does it mean that this version is correct?
I think we can document that the expected behavior is exactly how the new implementation behaves, and move on. I don't think this really makes a difference on anyone, since nobody is going to actually use something like \_ _ -> 42 as the combining function.
There was a problem hiding this comment.
If possible, it's also good to add a test which demonstrates this corner case.
| ({cpu: 4700174302334 | ||
| | mem: 4700000857208}) |
There was a problem hiding this comment.
9% CPU speedup and 12% MEM speedup for such a low-hanging fruit.
| ({cpu: 3700153208683 | ||
| | mem: 3700000726876}) |
There was a problem hiding this comment.
11.5% CPU speedup and 15% MEM speedup.
| ({cpu: 4100167271084 | ||
| | mem: 4100000808938}) |
There was a problem hiding this comment.
13.5% CPU speedup and 16.5% MEM speedup.
| ({cpu: 4100161979547 | ||
| | mem: 4100000772592}) |
There was a problem hiding this comment.
15% CPU speedup and 18% MEM speedup.
| That b -> f 0 b | ||
| These a b -> f a b | ||
| in Value (fmap (fmap unThese) combined) | ||
| unionWith f = coerce (Map.unionWith @CurrencySymbol (Map.unionWith @TokenName f)) |
There was a problem hiding this comment.
I'm not sure that's right. In the original, whenever there isn't a collision, the input function f is applied with the respective value and a 0. I don't see that happening in this new version, unless I'm reading the code wrong.
|
I'm closing this PR in favor of the #6656 issue. |
Addresses:
from #5967.