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
Remove all the debug traces from the vm #1198
Conversation
Codecov Report
Flags with carried forward coverage won't be shown. Click here to find out more. |
return '0x' + new BN(item).toString(16, 0) | ||
}) | ||
|
||
const name = eventObj.opcode.name |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is it possible to regenerate this data in the step
event? We have access to this eventObj
in the event right? Then it is OK if all this is removed in this context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, you can generate the same data.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This change here changes the structure of the step event as mentioned in #1199
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Where's the change? I really don't get it.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If one would listen to step
like this;
vm.on('step', (obj: any) => {
// do stuff
let stack = obj.hexStack
// do stuff with stack (is now undefined)
})
Then this code now does not work anymore, since hexStack
is not available anymore (and this opTrace
thing is also not available anymore).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That object is not passed to the step handler. Take a look at line 232:
return this._vm._emit('step', eventObj)
Only eventObj
is passed, which I didn't modify.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ok (short version, have a long version too 😄 , but not enough time right now).
I worked several hours on this Totally understand though that we needed to remove here asap due to timing considerations. |
We are receiving complains from our users about the release candidate version we published running >6x slower than our previous version.
The main difference between these versions is that the RC version uses v5, instead of v4, making it the first consumer of v5.
This performance regression is aligned with that @jochem-brouwer reported when experimenting with removing the traces and running the standard tests, so I instructed one of our users to remove them from their
node_modules
, and not only the performance regression was gone, but the test run actually faster.This PR removes all the debug traces from the VM. I didn't remove them from other packages to keep the PR small, and because we don't use them, so it's not as urgent.
@holgerd77, do you think we can get this published today? We really need to publish a final version of our Berlin support on Wednesday, or otherwise there won't be any way to test smart contracts against Berlin, and we can't publish something with a regression this significant.