vm: skip _runStepHook method if no step event listener

[acolytec3]

Adds a check in the interpreter to skip the _runStepHook function if there are no step event listeners since this method's sole purpose is to create and emit the step event. The step is only used in tests in our code base so this should produce some (likely minor) overall improvement in VM performance since this function is run on each step in VM execution.

[codecov]

Codecov Report

Merging #1676 (b2c8375) into master (06bdad5) will decrease coverage by 0.04%.
The diff coverage is 100.00%.

Flag	Coverage Δ
block	85.01% <ø> (ø)
blockchain	83.28% <ø> (ø)
client	71.22% <ø> (ø)
common	93.89% <ø> (ø)
devp2p	82.36% <ø> (-0.34%)	⬇️
ethash	90.76% <ø> (ø)
trie	86.18% <ø> (ø)
tx	89.94% <ø> (ø)
util	92.62% <ø> (ø)
vm	81.27% <100.00%> (+<0.01%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

[holgerd77]

Looks good, tested this with the following code:

import VM from './src'
let vm = new VM()
vm.listenerCount('step') // 0
vm.on('step', () => { console.log('Called step') })
vm.listenerCount('step') // 1

This should also be implicitly covered by tests in tests/api/events.spec.ts.

Read something about this in the chat but couldn't find any more, so was there actually some impact on performance by this change?

[holgerd77]

Argh, this can actually be directly reverted again.

The _runStepHook function is actually also used to prepare the output for the DEBUG logger, see here.

Could you guys please have a minimal look into the code base itself before you comment out some stuff? 😐 This is a bit annoying and a bit too sloppy to overlook.

[jochem-brouwer]

Argh, sorry this is my fault @holgerd77, I pitched this to @acolytec3 as "low hanging fruit" optimization and did skim over the code to convince myself there were no side effects if we disabled it with no listeners. Clearly I was not careful and did not take enough time to notice there were in fact side effects. My apologies! 😞

[holgerd77]

Thanks, no worries, nothing which can't be reverted. Generally a good idea.

Respectively, on looking at the code again: this just seems to need an additional or check for this._vm.DEBUG in the call-it-or-not switch. 🙂

[acolytec3]

Ugh, sorry, that wasn't obvious to me though I wondered about the flag. I'll add another PR tonight to address.