Coverage init no longer hoisted to the top in Babel 7.0.0-beta

[loganfsmyth]

Heya, just came across this so I figured I'd pass it along.

https://github.com/istanbuljs/istanbuljs/blob/2ce8974/packages/istanbul-lib-instrument/src/visitor.js#L519

is now breaking on Babel 7.x beta because we added a new blockHoist level 4 to work around some issues.

You'll need to bump that up for things to keep working.

That said, we've been really hoping that in the long run we can remove the concept of blockHoist. I don't know if it'll happen, but we'll see.

Have you all considered approaches that avoid the need for it? For instance if you use hoisting to your advantage and made that a function declaration and called it every time you needed the object, you could lazy-init the coverage object on first access and avoid the need got blockHoist.

[bcoe]

@loganfsmyth thanks for the heads up; in the short term we'll probably just bump up to 4 when we upgrade to Babel 7.

Would definitely entertain eliminating the need for blockHoist in the future -- but would definitely want to do some performance benchmarking to see if there's any noticeable slowdown to instrumentation or execution as a result of refactoring ... realistically, I also don't have too many cycles in the immediate future to take on this refactor unless someone else dives into it.

[loganfsmyth]

The blockHoist value has been increased in #135 so this issue could potentially be closed.

That said, it might be good to have this or another issue to track removing blockHoist in general. One downside of the current approach is that it's only 100% effective if Babel is the only tooling performing transformations. Now that Webpack has good support for ES6 module syntax for instance, fewer people are using Babel's compiled modules, which means that it isn't possible for _blockHoist in ensure that the cov structure is initialized before imports are executed. The same is true of native module support landing in browsers.

This means that it is possible for dependency cycles between files to run functions before the file coverage structure has been initialized.

My suggestion would still be to use a lazy-initialized function declaration to handle hoisting, e.g.

var COVERAGE_VAR = (function () {
    var path = PATH,
        hash = HASH,
        global = (new Function('return this'))(),
        gcv = GLOBAL_COVERAGE_VAR,
        coverageData = INITIAL,
        coverage = global[gcv] || (global[gcv] = {});
    if (coverage[path] && coverage[path].hash === hash) {
        return coverage[path];
    }
    coverageData.hash = hash;
    return coverage[path] = coverageData;
})();

could instead be

function COVERAGE_VAR() {
    var path = PATH,
        hash = HASH,
        global = (new Function('return this'))(),
        gcv = GLOBAL_COVERAGE_VAR,
        coverageData = INITIAL,
        coverage = global[gcv] || (global[gcv] = {});
    if (coverage[path] && coverage[path].hash === hash) {
        return coverage[path];
    }
    coverageData.hash = hash;
    coverage[path] = coverageData;

    COVERAGE_VAR = function() { return coverageData; };

    return coverageData;
}

with references being COVERAGE_VAR().foo instead of COVERAGE_VAR.foo or something, so the JIT would be able to very easily be able to inline the coverage after it is initialized.

[coreyfarrell]

I think your suggestion of converting the coverage variable into a function which returns the coverage data is actually a good idea for performance. I just ran a benchmark against the following script:

'use strict';

function a(b) {
	return b ? '1' : '2';
}


console.time('bench');
for (let i = 0; i < 1000000000; i++) {
	a();
}
console.timeEnd('bench');

I instrumented this script with npx nyc instrument --compact=false bench.js > bench-i1.js using nyc 14.1.1. I copied the instrumented result to bench-i2.js and manually changed the code to follow your suggestion.

I executed this using:

for i in {1..10}; do node bench.js; node bench-i1.js; node bench-i2.js; done

It took an average of 510ms to run bench.js, 3029ms to run bench-i1.js and 2954ms to run bench-i2.js. This is surprising to me but your suggested code change actually makes (this specific) instrumented code run about 1.3% faster. I think this could be due to our currently reference a var continuously. Changing the coverage variable to a const in the original instrumented code erases the difference, so I'm guessing that accessing a global scoped const or function is faster than accessing a var.