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

Evm gas cost stage 3 #1

Open
wants to merge 13 commits into
base: master
Choose a base branch
from
Open

Evm gas cost stage 3 #1

wants to merge 13 commits into from

Conversation

JacekGlen
Copy link
Collaborator

No description provided.

@JacekGlen JacekGlen requested a review from stan7123 June 8, 2023 13:31
Comment on lines 1 to 11
const { ArgumentParser } = require('argparse')
const { Benchmark } = require('benchmark')
const _ = require('lodash')
const { Block } = require('@ethereumjs/block')
const { Blockchain } = require('@ethereumjs/blockchain')
const { Common, Hardfork, ConsensusType, ConsensusAlgorithm } = require('@ethereumjs/common')
const { MemoryLevel } = require('memory-level')
const { EEI } = require('@ethereumjs/vm')
const { EVM } = require('@ethereumjs/evm')
const { DefaultStateManager } = require('@ethereumjs/statemanager')
const { Address, MAX_INTEGER_BIGINT, KECCAK256_RLP_ARRAY } = require('@ethereumjs/util')
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use TS imports rather than JS require:

Suggested change
const { ArgumentParser } = require('argparse')
const { Benchmark } = require('benchmark')
const _ = require('lodash')
const { Block } = require('@ethereumjs/block')
const { Blockchain } = require('@ethereumjs/blockchain')
const { Common, Hardfork, ConsensusType, ConsensusAlgorithm } = require('@ethereumjs/common')
const { MemoryLevel } = require('memory-level')
const { EEI } = require('@ethereumjs/vm')
const { EVM } = require('@ethereumjs/evm')
const { DefaultStateManager } = require('@ethereumjs/statemanager')
const { Address, MAX_INTEGER_BIGINT, KECCAK256_RLP_ARRAY } = require('@ethereumjs/util')
import { ArgumentParser } from 'argparse'
import benchmark from 'benchmark'
import _ from 'lodash'
import { Block } from '@ethereumjs/block'
import { Blockchain } from '@ethereumjs/blockchain'
import { Common, Hardfork, ConsensusType, ConsensusAlgorithm } from '@ethereumjs/common'
import { MemoryLevel } from 'memory-level'
import { EEI } from '@ethereumjs/vm'
import { EVM } from '@ethereumjs/evm'
import { DefaultStateManager } from '@ethereumjs/statemanager'
import { Address, MAX_INTEGER_BIGINT, KECCAK256_RLP_ARRAY } from '@ethereumjs/util'

Copy link
Collaborator

Choose a reason for hiding this comment

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

Done.
Although my IDE is showing some lines with red underline ;)

const { DefaultStateManager } = require('@ethereumjs/statemanager')
const { Address, MAX_INTEGER_BIGINT, KECCAK256_RLP_ARRAY } = require('@ethereumjs/util')

type Stats = {
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Declaring type seems unusual, class is more standard

Suggested change
type Stats = {
class Stats {

const { Address, MAX_INTEGER_BIGINT, KECCAK256_RLP_ARRAY } = require('@ethereumjs/util')

type Stats = {
run_id?: number
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Use camelCase which is consistent with the rest of the code base:

Suggested change
run_id?: number
runId?: number

promiseResolve = resolve
})

const bench = new Benchmark({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That becomes

Suggested change
const bench = new Benchmark({
const bench = new benchmark({

if you use imports

onCycle: (event: any) => {
stateManager.clearContractStorage(Address.zero())
},
minSamples: 1,
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

1 sample seems a bit low. Can we make it higher?

Copy link
Collaborator

Choose a reason for hiding this comment

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

Changed to default which is 5.

// maxTime: 5,
})
.on('complete', () => {
promiseResolve({
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That seems a bit convoluted, is there a reason for this?

If not, we can remove .on('complete') event and simply have something like this:

  bench.run()
  let result : Stats = {
      iterations_count: bench.count,
      total_time_ns: Math.round(bench.stats.mean * 1_000_000_000),
      std_dev_time_ns: Math.round(bench.stats.deviation * 1_000_000_000),
  }

  return result

Copy link
Collaborator

@stan7123 stan7123 Jun 13, 2023

Choose a reason for hiding this comment

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

The reason is that I want to wait for the async code to be benchmarked. The only way with benchmark.js is to wait for complete event to be emitted and then resolve the whole runBenchmark function.

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.

2 participants