go-nitro icon indicating copy to clipboard operation
go-nitro copied to clipboard

Remove duplicated code from `gas-benchmarks`

Open nksazonov opened this issue 3 years ago • 2 comments

However, I think one downside is now we have quite a bit of duplication:

  • gas.ts contains similar information to gasResults.json, only with comments and without meaningful numbers. There is potential for confusion here. Could you simply parse the existing gasResults.json instead of using an emptyGasResults? You may not even need to zero everything out, then (but you could).
  • exportBenchmarkResults.ts duplicates the comments from gas.ts, and is also duplicated with respect to benchmark.test.ts. When we want to add a new benchmark, it seems as if there are now quite a lot of files to update.

One way to improve this is to drop the benchmark:diff side of things. I imagine we won't use it so much -- and even in CI we can simply run npm run benchmark:update and let git diff --exit-code do the "check" on the gas numbers. What do you think?

Originally posted by @geoknee in #802.


You are right, we have quite a bit of duplication. Actually, the reason for both points you describe is to keep color-coded comparison results, which I believe is a powerful feature as it is very convenient to see the difference already in console, not needing to do math in your head.

It is also worth mentioning that exportBenchmarkResults.ts takes zeroed gas.ts, but not gasResults.json, as the latest may not exist.

So to keep the color-coded diff feature, we definitely need to keep benchmark.test.ts (otherwise we would need to explicitly print colorful messages to console ourselves). Thus, I can see several options how we can remove the duplication:

  1. Create a static Benchmark object, which will describe both benchmark result structure interface and benchmark method to invoke for that result.
{
    title: 'directlyFundAChannelWithETHSecond.satp',
    setup: async () => {
        const setupTX = nitroAdjudicator.deposit(MAGIC_ADDRESS_INDICATING_ETH, X.channelId, 0, 5, {
            value: 5,
        });
        await (await setupTX).wait();
    },
    method: async () => {
        await nitroAdjudicator.deposit(MAGIC_ADDRESS_INDICATING_ETH, X.channelId, 5, 5, {value: 5})
    }
}

Both test and script files will just traverse through all the field, invoking the setup, benchmarking the method and either testing for equality with or saving the results to title.

This way, the only place we would need to make a change to (if a new benchmark is introduced / old changed) is this Benchmark object.

Give up the script and proceed only with the test. Also introduce a flag (--write=false/true), which will control whether the test is updating gas results after it runs or not. To remove zeroed gas.ts, we could introduce a function to create an empty gas results object, which will be changed during benchmark test with --write=true.

Originally posted by @nksazonov in #802.

nksazonov avatar Jul 22 '22 07:07 nksazonov

I think it would be nice for us to be able to split up the benchmark code a bit into several files, if that isn't too much of a headache.

geoknee avatar Aug 25 '23 16:08 geoknee

Another idea which might make everything else easier / faster -- rewrite all of this in Go using a SimulatedBackend.

geoknee avatar Aug 25 '23 16:08 geoknee