-
Notifications
You must be signed in to change notification settings - Fork 5.9k
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
Yul: Avoid expensive operations in functions called very often #15574
Conversation
Benchmarking on my local laptop shows consistent improvement on external benchmarks: Before:
After:
|
3c8a332
to
846d615
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 think Add 2 is fine and we could investigate your suggestion of relocating it to a better place.
I am not sure about the changes related to Add 1. Do you have the benchmarks with only Add 2
?
assert(validLiteral(_lhs)); | ||
assert(validLiteral(_rhs)); |
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.
Is this really significant? validLiteral
is still being called twice.
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'd myself just have removed these entirely. Any assertion that we deem ok to just do non-release we might as well not do at all in our context. But whatever, no need to keep this waiting.
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.
Any assertion that we deem ok to just do non-release we might as well not do at all in our context.
I don't agree. The standard assert
macro is perfectly fine as a baseline for sanity checks and even when disabled still adds value by documenting the assumptions. Having asserts checked in release builds is definitely nice to have and saved us many times, but when they're too costly I'd still rather fall back to using assert
than remove them completely.
The method
validLiteral
is nontrivial and should not be called (twice!) on every call to the equality comparison.
It might be desirable to re-introduce the check in a more suitable place.
We're asserting validity here because ==
does not compare the whole LiteralValue
. This is fine for valid literals but not in general and I don't think that's obvious.
A better place for these checks would be the implementation of ==
, but that would introduce a circular dependency. And would not make it less costly anyway.
I don't have benchmarking results for the changes separately, although I could compute them if necessary. |
The equality comparison is called very often in CommonSubexpressionEliminator, presumably because it is the comparison operator used for the map `CommonSubexpressionEliminator::m_replacementCandidates`. The method `validLiteral` is nontrivial and should not be called (twice!) on every call to the equality comparison.
846d615
to
5150015
Compare
This is what it looks like for me: IR
Legacy
|
Oh, this almost suggests we should revert the second commit. |
Probably a fluke given what's in the PR. I would not expect it slow anything down so I'd not worry too much about it. The differences are still just a few percent. Unfortunately these numbers are not consistent enough to evaluate very small differences with a single run (at least on my hardware, @clonker seems to be getting better results). Running them multiple times and averaging helps. |
Two changes are proposed here:
std::regex
) whenever possible.Add 1: The equality comparison is called very often in
CommonSubexpressionEliminator
, presumably because it is the comparison operator used for the mapCommonSubexpressionEliminator::m_replacementCandidates
. The methodvalidLiteral
is nontrivial and should not be called (twice!) on every call to the equality comparison.It might be desirable to re-introduce the check in a more suitable place.
Add 2: Method
Parser::fetchDebugDataFromComment
was matching against a regex, but most of the time it was trying to match against an empty string.std::regex
is known to be extremely slow. This would not be such a problem if this method was not called after every parsed token, seeParser::advance
.I believe even better solution would be to move the call to
fetchDebugDataFromComment
to a more appropriate place; my intuition says it is currently misplaced.