-
Notifications
You must be signed in to change notification settings - Fork 5.7k
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
Make CommonSubExpression Eliminator consider gas costs as well as code size #15048
base: develop
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||||||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
@@ -29,6 +29,7 @@ | |||||||||||||||||||||||||||
#include <libevmasm/JumpdestRemover.h> | ||||||||||||||||||||||||||||
#include <libevmasm/BlockDeduplicator.h> | ||||||||||||||||||||||||||||
#include <libevmasm/ConstantOptimiser.h> | ||||||||||||||||||||||||||||
#include <libevmasm/GasMeter.h> | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
#include <liblangutil/CharStream.h> | ||||||||||||||||||||||||||||
#include <liblangutil/Exceptions.h> | ||||||||||||||||||||||||||||
|
@@ -784,6 +785,15 @@ std::map<u256, u256> const& Assembly::optimiseInternal( | |||||||||||||||||||||||||||
return _i == AssemblyItem{Instruction::MSIZE} || _i.type() == VerbatimBytecode; | ||||||||||||||||||||||||||||
}); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
auto runGas = [&](AssemblyItems const& items, KnownState _state) { | ||||||||||||||||||||||||||||
GasMeter gasMeter{std::make_shared<KnownState>(_state), _settings.evmVersion}; | ||||||||||||||||||||||||||||
GasMeter::GasConsumption gas; | ||||||||||||||||||||||||||||
for (auto const& item: items) | ||||||||||||||||||||||||||||
gas += gasMeter.estimateMax(item); | ||||||||||||||||||||||||||||
return gas; | ||||||||||||||||||||||||||||
}; | ||||||||||||||||||||||||||||
Comment on lines
+788
to
+794
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Your implementation ignores There's a correct implementation in solidity/libevmasm/Inliner.cpp Lines 47 to 59 in ebdce26
I think it would be to reuse it here too. It looks general enough that perhaps we could just move it to |
||||||||||||||||||||||||||||
auto runs = static_cast<bigint>(_settings.expectedExecutionsPerDeployment); | ||||||||||||||||||||||||||||
|
||||||||||||||||||||||||||||
auto iter = m_items.begin(); | ||||||||||||||||||||||||||||
while (iter != m_items.end()) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
|
@@ -796,7 +806,10 @@ std::map<u256, u256> const& Assembly::optimiseInternal( | |||||||||||||||||||||||||||
try | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
optimisedChunk = eliminator.getOptimizedItems(); | ||||||||||||||||||||||||||||
shouldReplace = (optimisedChunk.size() < static_cast<size_t>(iter - orig)); | ||||||||||||||||||||||||||||
solAssert(iter >= orig); | ||||||||||||||||||||||||||||
auto originalCost = static_cast<bigint>(iter - orig) + runs * runGas(AssemblyItems(orig, iter), eliminator.getKnownState()).value; | ||||||||||||||||||||||||||||
auto optimizedCost = static_cast<bigint>(optimisedChunk.size()) + runs * runGas(optimisedChunk, eliminator.getKnownState()).value; | ||||||||||||||||||||||||||||
shouldReplace = optimizedCost < originalCost; | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
catch (StackTooDeepException const&) | ||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
|
@@ -817,7 +830,9 @@ std::map<u256, u256> const& Assembly::optimiseInternal( | |||||||||||||||||||||||||||
else | ||||||||||||||||||||||||||||
copy(orig, iter, back_inserter(optimisedItems)); | ||||||||||||||||||||||||||||
} | ||||||||||||||||||||||||||||
if (optimisedItems.size() < m_items.size()) | ||||||||||||||||||||||||||||
auto optimisedItemsCost = static_cast<bigint>(optimisedItems.size()) + runs * runGas(optimisedItems, KnownState{}).value; | ||||||||||||||||||||||||||||
auto itemsCost = static_cast<bigint>(m_items.size()) + runs * runGas(m_items, KnownState{}).value; | ||||||||||||||||||||||||||||
Comment on lines
+833
to
+834
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. If we want to try and be accurate about this, the code size component of the cost should be calculated similarly to solidity/libevmasm/Inliner.cpp Line 178 in 272892e
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Yeah, we really should do that. I'd even go as far as to say that not doing it makes the calculation here broken. As is, the run cost will completely overshadow the data cost. For example with the standard settings of 200 runs and assuming non-creation data (cost: 200 gas per byte) we'd expect each byte count as much as each unit of gas used by the instruction. But in the current implementation the instruction cost will count 200x more, making the data cost almost insignificant. |
||||||||||||||||||||||||||||
if (optimisedItemsCost < itemsCost) | ||||||||||||||||||||||||||||
Comment on lines
-820
to
+835
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think that we should keep the old condition here. It seems to me that the purpose of the check was not to ensure that new items are not more expensive - that will always be the case - but just to avoid bumping In fact, the new condition could actually be wrong in some cases. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Actually, the way |
||||||||||||||||||||||||||||
{ | ||||||||||||||||||||||||||||
m_items = std::move(optimisedItems); | ||||||||||||||||||||||||||||
count++; | ||||||||||||||||||||||||||||
|
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
|
@@ -77,6 +77,8 @@ class CommonSubexpressionEliminator | |||||
/// @returns the resulting items after optimization. | ||||||
AssemblyItems getOptimizedItems(); | ||||||
|
||||||
KnownState const& getKnownState() const { return m_state; } | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
|
||||||
|
||||||
private: | ||||||
/// Feeds the item into the system for analysis. | ||||||
void feedItem(AssemblyItem const& _item, bool _copyItem = false); | ||||||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. You should add some cases to test that show that the runtime cost an the A test with something that gets an infinite estimate would not hurt either. |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -30,6 +30,7 @@ | |
#include <libevmasm/ControlFlowGraph.h> | ||
#include <libevmasm/BlockDeduplicator.h> | ||
#include <libevmasm/Assembly.h> | ||
#include <libevmasm/GasMeter.h> | ||
|
||
#include <boost/test/unit_test.hpp> | ||
|
||
|
@@ -111,8 +112,18 @@ namespace | |
iter = eliminator.feedItems(iter, _input.end(), usesMSize); | ||
bool shouldReplace = false; | ||
AssemblyItems optimisedChunk; | ||
auto runGas = [&](AssemblyItems const& items) { | ||
GasMeter gasMeter{std::make_shared<KnownState>(eliminator.getKnownState()), solidity::test::CommonOptions::get().evmVersion()}; | ||
GasMeter::GasConsumption gas; | ||
for (auto const& item: items) | ||
gas += gasMeter.estimateMax(item); | ||
return gas; | ||
}; | ||
optimisedChunk = eliminator.getOptimizedItems(); | ||
shouldReplace = (optimisedChunk.size() < static_cast<size_t>(iter - orig)); | ||
shouldReplace = ( | ||
optimisedChunk.size() < static_cast<size_t>(iter - orig) && | ||
!(runGas(AssemblyItems(orig, iter)) < runGas(optimisedChunk)) | ||
); | ||
Comment on lines
+123
to
+126
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This is not quite the same as actually happens in Assembly.cpp now, is it :-)? There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think we should extract this bit in There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Stop suggesting to refactor things. This is an intentional copy of There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Except that it no longer is, you just said that yourself above. I'm not suggesting a refactor for the sake of a refactor. I'm saying that due to this being a copy we're not testing the actual implementation here and IMO it's a problem if we're interested in it being correct. I mean, if the code is, as you say, obsolete then maybe we're not and maybe just recopying it is enough, but I don't think the suggestion was that outrageous. It shouldn't have been a copy to begin with. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Does this PR introduce a new copy of something that wasn't there before? Was the scope and intention of this PR fixing and refactoring the testing setup? It was neither. Hence what you're suggesting is extending the scope of the PR to something that's an entirely independent issue - and one at that, that we'd never individually have solved. That's the very definition of a distraction and wasting time. Not too outrageous in isolation, but this is a pattern that results in overall throughput being near zero. |
||
if (shouldReplace) | ||
optimisedItems += optimisedChunk; | ||
else | ||
|
@@ -1649,6 +1660,40 @@ BOOST_AUTO_TEST_CASE(cse_replace_too_large_shift) | |
}); | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(cse_dup) | ||
{ | ||
AssemblyItems input{ | ||
u256(0), | ||
Instruction::DUP1, | ||
Instruction::REVERT | ||
}; | ||
AssemblyItems output = input; | ||
|
||
checkCSE(input, output); | ||
checkFullCSE(input, output); | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(cse_push0) | ||
{ | ||
AssemblyItems input{ | ||
u256(0), | ||
u256(0), | ||
Instruction::REVERT | ||
}; | ||
AssemblyItems output{ | ||
u256(0), | ||
Instruction::DUP1, | ||
Instruction::REVERT | ||
}; | ||
// The CSE replaces with DUP1 PUSH0 because it does not take into account PUSH0 being cheaper | ||
checkCSE(input, output); | ||
|
||
// The full handling by the compiler (Assembly::optimiseInternal) does not replace | ||
// because it checks code size, which is equal, and gas cost, which is cheaper for PUSH0 | ||
output = input; | ||
checkFullCSE(input, output); | ||
} | ||
|
||
BOOST_AUTO_TEST_CASE(inliner) | ||
{ | ||
AssemblyItem jumpInto{Instruction::JUMP}; | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -33,8 +33,8 @@ contract C { | |
// f_which(uint256[],uint256[2],uint256): 0x40, 1, 2, 1, 5, 6 -> 0x20, 0x40, 5, 2 | ||
// f_which(uint256[],uint256[2],uint256): 0x40, 1, 2, 1 -> FAILURE | ||
// f_storage(uint256[],uint256[2]): 0x20, 1, 2 -> 0x20, 0x60, 0x20, 1, 2 | ||
// gas irOptimized: 111395 | ||
// gas legacy: 112709 | ||
// gas legacyOptimized: 111852 | ||
// gas irOptimized: 111642 | ||
// gas legacy: 112944 | ||
// gas legacyOptimized: 112090 | ||
Comment on lines
-36
to
+38
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This got more expensive. It's not supposed to happen with this kind of change, is it? |
||
// f_storage(uint256[],uint256[2]): 0x40, 1, 2, 5, 6 -> 0x20, 0x80, 0x20, 2, 5, 6 | ||
// f_storage(uint256[],uint256[2]): 0x40, 1, 2, 5 -> FAILURE |
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.
Are you sure you want to make a copy of the whole
KnownState
here? Two copies in fact, becausemake_shared()
will allocate another one.Also,
getKnownState()
should probably be returningshared_ptr
as well.