[WIP] Refactor Stack to eliminate mix types

[Bhargavasomu]

What was wrong?

As part of Issue #1656

How was it fixed?

The Stack now stores only objects of type int. It previously also used to store bytes. Now the bytes are converted to int before storing on the stack, and any the Stack issues out the popped elements in the form of int. It is expected to convert the integers to bytes at the call site, wherever necessary.

Cute Animal Picture

[Bhargavasomu]

@cburgdorf We can see the Stack performance comparison for the old version and the new version under py36-stack-benchmark. But I feel that just comparing the Stack Times is not a good benchmark, because we can see that performance decrease while pushing and increases while popping, when compared to the old one. So we might not still get a full idea of the performance change.

A better benchmark would be comparing the times taken to run a computation (like a benchmark bytecode) between the old and the new version. This way we would get an overall performance feel caused by the changes. Hence, where can I find a bunch of sample bytecodes to run the benchmarks against them? Are they somewhere in the fixtures directory?

[Bhargavasomu]

Some Points to Note

• There are lots of places in the code which has to be deleted, but were kept for running the benchmarks. Will delete them once confirmed with the procedure.
• I just implemented pop and pop_n functions instead of pop1, pop2, pop3 because there are places in the codebase which requires the popping of 5 elements, and I felt that this approach was not so feasible.
• And I have directly used the following method as it was faster than the other methods suggested by @pipermerriam. I have tested these on a sample stack. Will add this benchmark too if needed.

def pop3(self):
    try:
        return (self.values.pop(), self.values.pop(), self.values.pop())
    except IndexError:
        raise InsufficientStack("...")

[pipermerriam]

There are lots of places in the code which has to be deleted, but were kept for running the benchmarks. Will delete them once confirmed with the procedure.

To do this we should first write the benchmark code against master and get that merged. Then you can switch back and forth between your branch and master to compare benchmarks.

[pipermerriam]

I just implemented pop and pop_n functions instead of pop1, pop2, pop3 because there are places in the codebase which requires the popping of 5 elements, and I felt that this approach was not so feasible.

I'm still leaning pretty heavily towards just having pop1, pop2, for every number of elements that we need to pop. It may not be very DRY but it will be highly performant and shouldn't result in much of any code maintenance since it's unlikely to change. Each of these APIs would need to be replicated on the Computation object as well.

[pipermerriam]

With respect to benchmarking:

Your suggestion about benchmarking these at the Computation level seems like the right track. As I think you pointed out, the benchmarks in this pull request aren't likely to give us a very good picture of the performance. You might write a small smart contract that does something like the following.

contract TestStack {
  function doLotsOfPops() {
    uint v = 0
    for (uint i=0; i<100; i++) {
      v += 100
    }

It'll be worth examining the bytecode this produces and writing up some variants that use in-memory variables instead of a constant like 100. This approach should give you benchmark scripts that better match the real execution environment and which are not tied directly to the Stack object's API.

[pipermerriam]

I'd like to completely get rid of the type hinting and any conversion between int <-> bytes in both the Computation and Stack APIs, moving all conversion down into the opcode functions themselves.

For example:

py-evm/eth/vm/logic/memory.py

Lines 6 to 8 in dfe43ae

	def mstore(computation: BaseComputation) -> None:
	start_position = computation.stack_pop(type_hint=constants.UINT256)
	value = computation.stack_pop(type_hint=constants.BYTES)

This would become:

def mstore(computation: BaseComputation) -> None:
    start_position = computation.stack_pop(type_hint=constants.UINT256)
    raw_value = computation.stack_pop1()
    value = int_to_big_endian(raw_value)
    ...

[kclowes]

Closing, stale.