-
Notifications
You must be signed in to change notification settings - Fork 62
Use a map to implement litToArrayPreimage #740
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
base: main
Are you sure you want to change the base?
Use a map to implement litToArrayPreimage #740
Conversation
Can you provide some examples and measurements? |
Wait, doesn't Haskell do memoization of such pure functions by default? I though this will only be computed once and then memoized... maybe I am wrong? If not, maybe we could simply add What do you think? |
Haha, I'm an idiot. There is no such |
@blishko keccak is actually annoyingly slow and this thing needs to be recomputed every time if it's not memoized, which is likely pretty expensive. I just somehow thought this cannot possibly be non-memoized, but it seems like I was wrong.... |
src/EVM/Expr.hs
Outdated
litToKeccak :: Expr a -> Expr a | ||
litToKeccak e = mapExpr go e | ||
where | ||
go :: Expr a -> Expr a | ||
go orig@(Lit key) = case litToArrayPreimage key of | ||
go orig@(Lit key) = case litToArrayPreimage key of -- This now calls the optimized version |
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.
Let's remove the comment here, I think it's okay without.
Here is a profiling run from around 15 minutes of the exploration of a swap function where the 100% of time spent in the
And with the patch:
|
How can I reproduce this? |
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.
I think this will be good, but I think we can clean it up a bit.
src/EVM/Expr.hs
Outdated
-- | images of keccak(bytes32(x)) where 0 <= x < 256 | ||
preImages :: [(W256, Word8)] | ||
preImages = [(keccak' (word256Bytes . into $ i), i) | i <- [0..255]] | ||
|
||
-- | images of keccak(bytes32(x)) where 0 <= x < 256 | ||
preImageLookupMap :: Map.Map W256 (Word8, W256) | ||
preImageLookupMap = Map.fromList $ map (\(imageHash, originalId) -> (imageHash, (originalId, imageHash + fromInteger 255))) preImages |
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.
I don't think we need to store imageHash + 255
in the value.
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.
Removed!
I'm coming back to this, sorry for the delay |
All tests are passing, so feel free to take another look if needed. Should I update the changelog about it? |
Can you please rebase on top of main? I would like to run a bit of benchmarking. I don't think changelog entry is necessary for this change. |
0614a7c
to
fc6ead7
Compare
Description
In certain runs,
litToArrayPreimage
was taking most of the execution time so this is an optimized version of it using mapsChecklist