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.

Sign up for GitHub

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

Jump to bottom

EVM: Memory Fix & Other Optimizations #2570

Merged
merged 6 commits into from
Mar 7, 2023
Merged

EVM: Memory Fix & Other Optimizations #2570

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
0a1abc0
EVM: Rename evm debug logger to evm:evm (one for package, one for cla…
holgerd77 Mar 6, 2023
8ab8758
VM: Rename message checkpoint to state checkpoint in debug message (t…
holgerd77 Mar 6, 2023
348561e
EVM: CALL/CREATE debug exit msg differentiation
holgerd77 Mar 6, 2023
f2d046a
EVM: avoid buffer copy in memory read (performance)
holgerd77 Mar 6, 2023
b147be1
EVM: Rewrite runCall() checkpoint/revert conditional for readability/…
holgerd77 Mar 6, 2023
bc8bce8
EVM: Added EIP check for transient storage checkpointing
holgerd77 Mar 6, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
42 changes: 16 additions & 26 deletions packages/evm/src/evm.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -40,7 +40,7 @@ import type {
} from './types'
import type { Account } from '@ethereumjs/util'

const debug = createDebugLogger('evm')
const debug = createDebugLogger('evm:evm')
const debugGas = createDebugLogger('evm:gas')

// very ugly way to detect if we are running in a browser
Expand Down Expand Up @@ -364,13 +364,13 @@ export class EVM implements EVMInterface {
if (!message.code || message.code.length === 0) {
exit = true
if (this.DEBUG) {
debug(`Exit early on no code`)
debug(`Exit early on no code (CALL)`)
}
}
if (errorMessage !== undefined) {
exit = true
if (this.DEBUG) {
debug(`Exit early on value transfer overflowed`)
debug(`Exit early on value transfer overflowed (CALL)`)
}
}
if (exit) {
Expand Down Expand Up @@ -482,13 +482,13 @@ export class EVM implements EVMInterface {
if (message.code === undefined || message.code.length === 0) {
exit = true
if (this.DEBUG) {
debug(`Exit early on no code`)
debug(`Exit early on no code (CREATE)`)
}
}
if (errorMessage !== undefined) {
exit = true
if (this.DEBUG) {
debug(`Exit early on value transfer overflowed`)
debug(`Exit early on value transfer overflowed (CREATE)`)
}
}
if (exit) {
Expand Down Expand Up @@ -745,7 +745,7 @@ export class EVM implements EVMInterface {
}

await this.eei.checkpoint()
this._transientStorage.checkpoint()
if (this._common.isActivatedEIP(1153)) this._transientStorage.checkpoint()
if (this.DEBUG) {
debug('-'.repeat(100))
debug(`message checkpoint`)
Expand Down Expand Up @@ -789,29 +789,19 @@ export class EVM implements EVMInterface {
result.execResult.selfdestruct = {}
result.execResult.gasRefund = BigInt(0)
}
if (err) {
if (
this._common.gteHardfork(Hardfork.Homestead) ||
err.error !== ERROR.CODESTORE_OUT_OF_GAS
) {
result.execResult.logs = []
await this.eei.revert()
this._transientStorage.revert()
if (this.DEBUG) {
debug(`message checkpoint reverted`)
}
} else {
// we are in chainstart and the error was the code deposit error
// we do like nothing happened.
await this.eei.commit()
this._transientStorage.commit()
if (this.DEBUG) {
debug(`message checkpoint committed`)
}
if (
err &&
!(this._common.hardfork() === Hardfork.Chainstart && err.error === ERROR.CODESTORE_OUT_OF_GAS)
) {
result.execResult.logs = []
await this.eei.revert()
if (this._common.isActivatedEIP(1153)) this._transientStorage.revert()
if (this.DEBUG) {
debug(`message checkpoint reverted`)
}
} else {
await this.eei.commit()
this._transientStorage.commit()
if (this._common.isActivatedEIP(1153)) this._transientStorage.commit()
if (this.DEBUG) {
debug(`message checkpoint committed`)
}
Expand Down
2 changes: 1 addition & 1 deletion packages/evm/src/memory.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,7 @@ export class Memory {
const returnBuffer = Buffer.allocUnsafe(size)
// Copy the stored "buffer" from memory into the return Buffer

const loaded = Buffer.from(this._store.slice(offset, offset + size))
const loaded = this._store.slice(offset, offset + size)
Copy link
Member

Choose a reason for hiding this comment

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

Had to explicitly check that this could not lead to unwanted writes:

  • slice is a view point, so if you edit this buffer, you edit the "original" buffer too
  • loaded is not written to in this context
  • The returnBuffer is filled, and this actually copies the value (so if you edit it, it does not change the original)

So this is great :)

Copy link
Member Author

@holgerd77 holgerd77 Mar 7, 2023 •
edited

Choose a reason for hiding this comment

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

This is half-great. Martin (Holst Swende) explicitly said, that one of the core things we are doing not optimal in our VM is that we copy things around, while we should use the original pointer to memory (the test in the performance test suite this PR/change here half-fixes was actually that there was huge call data created in the test and this is then copied over and over on each repeated call).

So we should rather make sure that we also remove this loaded copy over time and double check that there are just no writes happening. This will eventually/likely give another huge/significant speed bump if we do this right.

Independent from the approval on this PR, since this only eliminates one unnecessary copy. So safe anyhow. 🙂

Will merge in.

Copy link
Member Author

Choose a reason for hiding this comment

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

(updated the comment above for better readability)

Copy link
Member

Choose a reason for hiding this comment

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

I think this is fine, loaded is not a copy, it is a view, so this should be a super fast operation.

Copy link
Member Author

Choose a reason for hiding this comment

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

Yeah, ok, but then latest the returnBuffer will copy this over, right?

So this is what we should avoid. Will play around with this a bit over the next days. So normally there is is no reason anyhow why memory read should be manipulated, I will go a bit through the places and see what is happening where. Maybe we can also do/solve/tackle this by social (dev) consensus, and just name all variables which still (can) contain memory pointers in some special way, something like callDataMemoryPointer or whatever, so that everyone knows that these need to be handled with special care.

And for this 0-filling, maybe we can do this later in the pipeline on the places where this is needed (can't completely remember, I think you implemented/added this at some point?).

Just first round ideas, but baseline would be that we need to get rid of this as Martin suggested. This is what he wrote (already over a year ago):

Still, the base problem is that you're always copying data in calls. That's not paid for (evm doesn't charge per byte in calls), so you shouldn't do it. Instead, treat the input as read-only memory and use the original backing data

Copy link
Member Author

Choose a reason for hiding this comment

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

Update: just did the test, these are the numbers of the performance tests when we remove this returnBuffer copy (this is not out of the box working yet though and needs subsequent fixes, tested on some mainnet blocks).

So these are the new values:

grafik

And these are the old ones:

grafik

Copy link
Member

Choose a reason for hiding this comment

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

Ah, you are absolutely right regarding this copying, I now see the point. I will give this a look soon also, this does not seem trivial but I also do not think it will be super complex either.

returnBuffer.fill(loaded, 0, loaded.length)

if (loaded.length < size) {
Expand Down
6 changes: 3 additions & 3 deletions packages/vm/src/eei/vmState.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -68,7 +68,7 @@ export class VmState implements EVMStateAccess {

if (this.DEBUG) {
this._debug('-'.repeat(100))
this._debug(`message checkpoint`)
this._debug(`state checkpoint`)
}
}

Expand All @@ -89,7 +89,7 @@ export class VmState implements EVMStateAccess {
}

if (this.DEBUG) {
this._debug(`message checkpoint committed`)
this._debug(`state checkpoint committed`)
}
}

Expand Down Expand Up @@ -127,7 +127,7 @@ export class VmState implements EVMStateAccess {
}

if (this.DEBUG) {
this._debug(`message checkpoint reverted`)
this._debug(`state checkpoint reverted`)
}
}

Expand Down