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

Benchmarking external tests #12441

Merged
merged 6 commits into from
Feb 14, 2022
Merged

Benchmarking external tests #12441

merged 6 commits into from
Feb 14, 2022

Conversation

cameel
Copy link
Member

@cameel cameel commented Dec 20, 2021

Depends on #12440. Merged.
Depends on #12532. Merged.
Depends on #12629. Merged.

This PR adds a mechanism for gathering a set of metrics from all external tests and combining them into an easily diffable JSON report.

Currently it gathers the following information:

  1. Gas usage as reported by the eth-gas-reporter plugin for Hardhat and Truffle. The plugin watches all function calls and contract deployments during test execution, measures used gas and presents results in an RST table.
  2. Bytecode size extracted from Truffle and Hardhat artifacts with jq.

The biggest part (report creation) is already implemented but there are still a few small things to iron out:

  • Make sure the plugin works and actually produces reports in all external projects.
  • Skipping the report in compile-only runs.
  • Attaching reports as artifacts in CI.
  • Collector job to gather all artifacts and combine them into a single JSON file.
  • Simple command to process the combined JSON report into a short summary containing only totals.

@cameel cameel added testing 🔨 has dependencies The PR depends on other PRs that must be merged first labels Dec 20, 2021
@cameel cameel self-assigned this Dec 20, 2021
@cameel cameel marked this pull request as draft December 20, 2021 21:02
@cameel cameel force-pushed the preset-selection-in-ext-tests branch from b1ce656 to 37f2c54 Compare December 21, 2021 13:13
@cameel cameel force-pushed the preset-selection-in-ext-tests branch from 37f2c54 to e22f0cf Compare December 22, 2021 12:06
@cameel cameel force-pushed the benchmarking-ext-tests branch 2 times, most recently from 7a5c835 to 20c256d Compare December 22, 2021 12:36
@cameel cameel force-pushed the preset-selection-in-ext-tests branch from e22f0cf to c40a44f Compare December 22, 2021 15:41
@cameel cameel force-pushed the benchmarking-ext-tests branch 8 times, most recently from 9280182 to 7864565 Compare December 22, 2021 22:56
@cameel
Copy link
Member Author

cameel commented Dec 22, 2021

This is pretty much done now.

Benchmark results for Gnosis are still incomplete, probably because its tests are not based on mocha but they will work after the switch to Hardhat in #12195 so there's no point fixing that here. For Colony I'm not sure if they're complete but it only runs on nightly so I'm leaving it as is for now. We should update it to the latest version first anyway.

When reviewing check the artifacts of the c_ext_benchmarks job. This is the primary output we'll use for benchmarking.

@cameel cameel marked this pull request as ready for review December 22, 2021 23:03
@cameel cameel force-pushed the preset-selection-in-ext-tests branch from c40a44f to 1928b78 Compare January 10, 2022 13:44
Base automatically changed from preset-selection-in-ext-tests to develop January 10, 2022 20:15
@cameel cameel mentioned this pull request Jan 12, 2022
10 tasks
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Jan 18, 2022
@cameel cameel marked this pull request as draft January 18, 2022 00:41
@cameel cameel force-pushed the benchmarking-ext-tests branch 2 times, most recently from 14ed1af to 4c76add Compare January 19, 2022 19:07
@cameel
Copy link
Member Author

cameel commented Jan 21, 2022

Rebased on the Uniswap PR (#12532) since that's likely to get merged soon and that way I can already include it here..

Base automatically changed from uniswap-ext-test to develop January 21, 2022 21:03
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Jan 21, 2022
@cameel cameel force-pushed the benchmarking-ext-tests branch 2 times, most recently from f765830 to 0ee4835 Compare January 24, 2022 13:04
@cameel cameel added the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2022
@cameel cameel changed the base branch from develop to reenable-bleeps-ext-test-without-governor-test February 4, 2022 14:24
Base automatically changed from reenable-bleeps-ext-test-without-governor-test to develop February 4, 2022 14:51
@cameel cameel removed the has dependencies The PR depends on other PRs that must be merged first label Feb 4, 2022
@cameel
Copy link
Member Author

cameel commented Feb 4, 2022

I updated the code for new external tests. This should now pass CI.

I also fixed an issue with benchmark results being missing for PRBMath and Bleeps. It looks like running tests via mocha suppresses the output from hardhat-gas-reporter. I had to change it so that tests are executed directly with hardhat test.

Unfortunately results seem incomplete in both cases. In Bleeps the table has no deployment costs while in PRBMath method call costs are missing. I'm not sure why so I asked for help in PaulRBerg/prb-math#70.

In any case, I think we should merge it even in the current state. Benchmark results are just extra information and jobs are on purpose designed in such a way that not having them does not make them fail.

@cameel
Copy link
Member Author

cameel commented Feb 4, 2022

Regarding no calls in PRBMath - mystery solved. All the methods called in tests are pure or view so there's no actual transaction happening and and eth_call does not report gas usage.

There are plans to make hardhat-gas-reporter inject eth_estimageGas instead and once that happens, we should automatically start seeing gas values for these methods in our benchmarks.

@cameel
Copy link
Member Author

cameel commented Feb 4, 2022

As for Bleeps, the issue is caused by the use of hardhat-deploy plugin. It deploys the contracts before tests are executed and hardhat-gas-reporter does not monitor these calls. The project actually has a separate command to report deployment gas. There's an issue to fix that in hardhat-gas-reporter though: cgewecke/hardhat-gas-reporter#86. So this is another thing that should just work once it's fixed upstream.

Copy link
Member

@ekpyron ekpyron left a comment

Choose a reason for hiding this comment

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

I vote for just merging this and seeing how it performs.
I by far have not done a complete line-by-line review, but since
(1) all of this is purely informative and non-critical, yet also (2) the information
will be quite helpful and useful, I think it may be reasonable to merge
and see how well it works without a full rigorous review.

@leonardoalt leonardoalt merged commit 947a599 into develop Feb 14, 2022
@leonardoalt leonardoalt deleted the benchmarking-ext-tests branch February 14, 2022 19:14
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants