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

feat: add op counter #24

Merged
merged 15 commits into from
Feb 14, 2024
Merged

Conversation

DoTheBestToGetTheBest
Copy link
Contributor

related with #20

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

@sslivkoff what should opcode_gas

the total gas cost per opcode * count?

src/counter_op.rs Outdated Show resolved Hide resolved
src/counter_op.rs Outdated Show resolved Hide resolved
@DoTheBestToGetTheBest
Copy link
Contributor Author

@mattsse does it look good to you ?

@DoTheBestToGetTheBest
Copy link
Contributor Author

the total gas cost per opcode * count?

I was thinking of mapping each opcode to its constant gas value. Then, when the final opcode result is mapped, I would simply multiply the constant gas by the number of repetitions of that opcode. but this implementation seems inefficient, doesn't it ?

@DoTheBestToGetTheBest
Copy link
Contributor Author

or it may be some funciton that can calculate the gas?

@sslivkoff what should opcode_gas

the total gas cost per opcode * count?

i tried to use the look up table from revm but it doesn't work it return for some opcode gas 0 and for some good value and for others false value

   fn step(&mut self, interp: &mut Interpreter, _context: &mut EvmContext<DB>) {
   
        let opcode_val = interp.current_opcode();
        let opcode =
            OpCode::new(opcode_val).unwrap_or_else(|| unsafe { OpCode::new_unchecked(opcode_val) });

  
        *self.opcode_counts.entry(opcode).or_insert(0) += 1;

        let spec_id = revm::primitives::specification::SpecId::SHANGHAI;
        let gas_info =
            revm::interpreter::instructions::opcode::spec_opcode_gas(spec_id)[opcode_val as usize];

        let opcode_gas = gas_info.get_gas() as u64;

        *self.opcode_gas.entry(opcode).or_insert(0) += opcode_gas;
    }
}```

@sslivkoff
Copy link
Member

@sslivkoff what should opcode_gas

the total gas cost per opcode * count?

afaui the same op-code can cost different amounts of gas depending on context and depending on input arguments

so ideally it would be the sum of each opcode ocurrence but this would not be a simple multiplication

@sslivkoff
Copy link
Member

there's also a bit of complication on contract calls returning whatever amount of spare unused gas they do not use

ideally want a situation where the sum of opcode gas entries equals the total amount of gas used by the transaction

might need to do some interesting nested gas accounting to make this happen given the various types of gas refunds

src/counter_op.rs Outdated Show resolved Hide resolved
src/counter_op.rs Outdated Show resolved Hide resolved
@mattsse
Copy link
Contributor

mattsse commented Feb 13, 2024

let's also make the fields pub so they can be consumed easily

@DoTheBestToGetTheBest
Copy link
Contributor Author

DoTheBestToGetTheBest commented Feb 14, 2024

let's also make the fields pub so they can be consumed easily

no problem!

@DoTheBestToGetTheBest
Copy link
Contributor Author

DoTheBestToGetTheBest commented Feb 14, 2024

@mattsse does it look goods to you ?

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

I few nits,

this could also use a helper function that returns an iterator over all opcodes and their count+gas (opcode, (u64,u64))

src/lib.rs Outdated Show resolved Hide resolved
src/counter_op.rs Outdated Show resolved Hide resolved
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
Co-authored-by: Matthias Seitz <matthias.seitz@outlook.de>
@DoTheBestToGetTheBest
Copy link
Contributor Author

I few nits,

this could also use a helper function that returns an iterator over all opcodes and their count+gas (opcode, (u64,u64))

thanks, i hope this time look good to you :)

Copy link
Contributor

@mattsse mattsse left a comment

Choose a reason for hiding this comment

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

lgtm

@mattsse mattsse merged commit e00355d into paradigmxyz:main Feb 14, 2024
11 checks passed
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