-
Notifications
You must be signed in to change notification settings - Fork 1.5k
feat: add precompile cache for execution #15536
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
Conversation
93f2f29
to
24a6c4e
Compare
CodSpeed Performance ReportMerging #15536 will not alter performanceComparing Summary
|
24a6c4e
to
eba460e
Compare
953907e
to
cbafb50
Compare
9552e8e
to
ef9cf9b
Compare
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 see any logical errors, LGTM! I think this works.
Would be nice to get rid of std
feature propagated everywhere, but not sure how to better approach it.
c936218
to
d1deb8e
Compare
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.
last nit
let key = | ||
PrecompileKey { address: *address, spec: self.spec, input: inputs.input.clone() }; | ||
|
||
let cache_result = cache.cache.get(&key); |
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.
we can avoid his because this is relatively large overhead per call
this is how the underlying precompiles does it:
let Some(precompile) = self.precompiles.get(address) else {
return Ok(None);
};
which is just .contains
so what we should do here is short-circuit if the precompiles don't contain the address
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 find the cache handling still a bit weird, because a bit confusing but for now this seems okay
Closes #13984