Gas opt: skip remaining calculation when not necessary
#448
Conversation
contracts/lib/AmountDeriver.sol
Outdated
| @@ -53,6 +51,9 @@ contract AmountDeriver is AmountDerivationErrors { | |||
| } | |||
| } | |||
|
|
|||
| // Derive time remaining until order expires | |||
| uint256 remaining = duration - elapsed; | |||
There was a problem hiding this comment.
TODO: could this potentially be unchecked? Still need to investigate
There was a problem hiding this comment.
Constraints that are true from c4 review:
$startTime \ge block.timestamp_t$ $endTime > block.timestamp_t$ -
$endTime > startTime$ (1, 2) $duration = endTime - startTime$ -
$duration > 0$ (3, 4) $elapsed = block.timestamp_w - startTime$ $elapsed \ge 0$ $block.timestamp = endTime \implies invalid(order)$ -
$elapsed < duration$ (6, 8)
From this I think unchecked is fine.
|
After a better look, this could even be further optimized, by sending directly However, this would either require changing |
|
For ref, see https://github.com/ProjectOpenSea/seaport/pull/355/files which is doing similar things. |
|
Now defering computation of For now deferred the decision of adding |
contracts/lib/AmountDeriver.sol
Outdated
| uint256 duration = endTime - startTime; | ||
|
|
||
| // Derive time elapsed since the order started & place on stack. | ||
| uint256 elapsed = block.timestamp - startTime; |
There was a problem hiding this comment.
this ended up making the change much bigger than expected, since changing the function from pure to view and relying on current timestamp force me to switch around a lot of code in the test suite
a better compromise could have been to instead keep the function pure and send in an extra currentTime. Someone please let me know if that's preferable, and I'll make the change
There was a problem hiding this comment.
I think deferring these calculations until necessary makes a lot of sense. If you add a section to the natspec here documenting the conditions asserted in _verifyTime you could also make this computation unchecked
contracts/lib/AmountDeriver.sol
Outdated
| uint256 duration = endTime - startTime; | ||
|
|
||
| // Derive time elapsed since the order started & place on stack. | ||
| uint256 elapsed = block.timestamp - startTime; |
There was a problem hiding this comment.
I think deferring these calculations until necessary makes a lot of sense. If you add a section to the natspec here documenting the conditions asserted in _verifyTime you could also make this computation unchecked
|
We do need to work out why these forge tests are breaking now as well |
|
@0age fixed all pending comments, and included underflow noticed as requested (actually re-used the notice from #355) as for the test suite, it was failing to compile due to a "stack too deep error", which I fixed with this. However, the forge suite is failing for me locally, for reasons I haven't yet understood. It fails on |
|
Rerunning forge tests now! |
|
While we're working out what's going on with the forge tests, just wanted to also flag an implication of this change: for orders with multiple ascending / descending amount items, these parameters will now be calculated multiple times. It does seem like most items won't have ascending or descending amounts, and the orders that do have an item with ascending or descending amounts usually won't have more than one, but just wanted to flag it and hear what you think. |
Motivation
Critical code paths for order fulfillment are preemptively computing
duration,elapsedandremainingtimestamps for all orders.In particular,
remainingcan always be derived from the other two, and it's only actually necessary whenstartAmount != endAmount. For static-price orders, it ends up never actually being used (both_locateCurrentAmountand_applyFractionbranch out and never use it).This opens the door to a couple of optimizations:
durationandelapsedwhen necessarystartAmount == endAmount, which should be a significant number of ordersyarn coverageof the relevant section (before and after shown below) shows gains of 130 to 160 gas for allfulfill*andmatch*calls.TODO:
main:
patched: