Skip to content
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

Gas report #104

Merged
merged 9 commits into from
Aug 19, 2023
Merged

Gas report #104

merged 9 commits into from
Aug 19, 2023

Conversation

vdrg
Copy link
Contributor

@vdrg vdrg commented Apr 11, 2023

No description provided.

@vdrg vdrg changed the base branch from main to develop April 11, 2023 21:26
bytes32 slot = keccak256(bytes("vulcan.ctx.gasReport.name"));
accounts.setStorage(address(vulcan.hevm), slot, b32Name);
bytes32 valueSlot = keccak256(abi.encodePacked("vulcan.ctx.gasReport", b32Name));
accounts.setStorage(address(vulcan.hevm), valueSlot, bytes32(gasleft()));
Copy link

@holic holic Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

ooc do these operations affect gasleft calculation? I assume gasleft() is computed first on this line, then put in storage, so I'm wondering if this storage operation would be the first gas consumed in this report?

(I am totally fine with this for the purposes of #103 but just curious!)

for (uint256 i = 0; i < 5; i++) {
new MockTarget();
}
ctx.endGasReport();
Copy link

@holic holic Apr 16, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

re: above, I'd be curious to see a similar measurement done without the context methods to see how they compare gas-wise (to determine if and how much gas is added by the gas reporting calls themselves), e.g.

uint256 startGas = gasleft();
for (uint256 i = 0; i < 5; i++) {
    new MockTarget();
}
println("gas used:", startGas - gasleft());

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry for the delay, we are in the middle of an audit right now, but will definitely look into this next week

Copy link
Contributor

@gnkz gnkz May 22, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hey @holic . This is the gas comparison between the vulcan method vs using gasleft

| test/_modules/Context.t.sol:GasReport contract |                 |        |        |        |         |
|------------------------------------------------|-----------------|--------|--------|--------|---------|
| Deployment Cost                                | Deployment Size |        |        |        |         |
| 488124                                         | 2470            |        |        |        |         |
| Function Name                                  | min             | avg    | median | max    | # calls |
| nativeReport                                   | 305575          | 305575 | 305575 | 305575 | 1       |
| vulcanReport                                   | 313671          | 313671 | 313671 | 313671 | 1       |

And these are the measurements:

  • Vulcan: 304141
  • Native: 303650

This is the contract we used to compare

contract GasReport {
    function vulcanReport() public {
        ctx.startGasReport("test");
        for (uint256 i = 0; i < 5; i++) {
            new MockTarget();
        }
        ctx.endGasReport();
    }

    function nativeReport() public {
        uint256 start = gasleft();
        for (uint256 i = 0; i < 5; i++) {
            new MockTarget();
        }
        println(string.concat("gas(test)", strings.toString(start - gasleft())));
    }
}

@holic
Copy link

holic commented May 10, 2023

Did ya'll have a chance to revisit this?

@gnkz gnkz marked this pull request as ready for review July 28, 2023 16:27
@gnkz gnkz merged commit f011bb7 into develop Aug 19, 2023
2 checks passed
@gnkz gnkz deleted the gas-report branch August 22, 2023 13:18
@gnkz gnkz mentioned this pull request Aug 22, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants