Skip to content

core/vm: jit vm#1490

Merged
obscuren merged 3 commits intoethereum:developfrom
obscuren:jit-vm
Aug 8, 2015
Merged

core/vm: jit vm#1490
obscuren merged 3 commits intoethereum:developfrom
obscuren:jit-vm

Conversation

@obscuren
Copy link
Copy Markdown
Contributor

  • Stage 1: (this PR) This will be the base for the improvements that will soon begin on the VM. This PR optimised the VM slightly by compiling byte code to instructions (function calls) and removes the need for random access in the byte-code. When the regular VM kicks in it checks if a compiled version of the contract (identified by H(code)) is available and if so will use the compiled version instead. If no compiled version is available it will compile the byte code in a separate thread and continues using the byte-code VM.
  • Stage 2: during byte-code VM operations periodically check if the compilation of the compiled contract has finished and if so move the execution over.
  • Stage 3: implement segmentation core/vm: code segmenting #1324

Status

stage 1 implemented
stage 2 implemented, but commented out
stage 3 TODO (future PTR)

Comment thread cmd/evm/jit.log Outdated
Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

oops :)

@obscuren obscuren force-pushed the jit-vm branch 7 times, most recently from 26a8837 to f7fcddf Compare July 27, 2015 07:56
@obscuren obscuren force-pushed the jit-vm branch 8 times, most recently from baded05 to 3eda45c Compare July 30, 2015 09:17
@obscuren obscuren added this to the 1.0.1 milestone Jul 30, 2015
@obscuren obscuren self-assigned this Jul 30, 2015
@obscuren obscuren force-pushed the jit-vm branch 4 times, most recently from 7680af3 to 8b370f1 Compare July 31, 2015 11:03
@obscuren obscuren force-pushed the jit-vm branch 4 times, most recently from a4e0b5c to 347b726 Compare August 2, 2015 18:51
@obscuren obscuren added the review label Aug 3, 2015
@obscuren obscuren modified the milestones: 1.0.2, 1.0.1 Aug 3, 2015
Comment thread Makefile Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Why is this being changed?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Because the GOROOT should determine the go version. Not PATH. Got a better suggestion?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

                                                                                  GOROOT is not always present in the environment.Using it here would require that people set it before building.                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                          ‎

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I am guessing that you added this in order to be able to build/test with different Go versions.
If that's what you want, we can add a variable GO that allows you to specify the go binary.
You could then invoke make as follows:

make GO=/path/to/go/bin/go1.5
// or even
make GO=go1.5

Comment thread core/vm/instructions.go Outdated
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I don't think it's possible to use a single shared base because it is pushed to the stack.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Sorry, that was a bit misguided. We also push common.BigFalse and friends to the stack in other places.
Bigints on the stack are not mutated. I think what @Gustav-Simonsson meant is that we could do something
like this to avoid allocating base when it is not required.

x, y := S256(stack.pop()), S256(stack.pop())
if y.Cmp(common.Big0) == 0 {
    stack.Push(common.Big0)
    return
}
...

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We can, yes. I don't think it will make much difference but certainly agree that not requiring base in certain areas is cleaner

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Actually the reason why it is as it is right now is because the code has been copied from the origin VM to their respective functions. I wanted minimal changes to the opcodes before "moving on" to reduce the risk of bugs and errors. Again I certainly agree it can and should be refactored.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

btw x.Abs(x) mutates the stack value if x < (1<<255).
We might want to look into that. Can this happen?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

ahh, no, it shouldn't happen because the stack never contains negative values. S256 always creates a new int if the number is negative, so Abs is fine then. But this is really hard to see.

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

btw this is a general potential danger with the current impl, if somewhere somehow one can push a negative int to the stack, it can be exploited by a number of other op codes.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

We need better big int objects (proper unsigned ints really). It shouldn't happen unless someone screws it up now

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think one way to make this safer would be to move S256 to a method on stack, e.g. popS256 and document under which conditions the value refers to the live int on the stack.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Hmm, interesting thought. Maybe we should remove push and add pushU256 and a pushS256. Remove all manual S/U256 before the push operation.

Comment thread core/vm/jit.go
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Hmm, we could potentially avoid this hashing by reusing the contract address as id, but then it's not unique on code. Which could be fine if most contracts use CALLCODE on existing contracts for the heavy lifting. What do you think?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I think the sha3 is fine for now. It's not that slow.

@obscuren obscuren force-pushed the jit-vm branch 4 times, most recently from 5f87691 to 94fd83e Compare August 7, 2015 10:49
* changed stack and removed stack ptr. Let go decide on slice reuse.
Reduced the amount of state copied that are required by N calls by doing
a balance check prior to any state modifications.
Reduced big int allocation by making stack items modifiable. Instead of
adding items such as `common.Big0` to the stack, `new(big.Int)` is
added instead. One must expect that any item that is added to the stack
might change.
Comment thread cmd/utils/flags.go
Copy link
Copy Markdown

Choose a reason for hiding this comment

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

JIT should be in caps.

obscuren added a commit that referenced this pull request Aug 8, 2015
@obscuren obscuren merged commit c93f0b9 into ethereum:develop Aug 8, 2015
@obscuren obscuren removed the review label Aug 8, 2015
@obscuren obscuren deleted the jit-vm branch August 8, 2015 13:36
tony-ricciardi pushed a commit to tony-ricciardi/go-ethereum that referenced this pull request Jan 20, 2022
maoueh pushed a commit to streamingfast/go-ethereum that referenced this pull request May 8, 2023
sduchesneau pushed a commit to streamingfast/go-ethereum that referenced this pull request May 16, 2025
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