When Babel 6 was released, I quickly realised that I kind of missed my target: tail call optimisation had been dropped in the process. But all was not lost, I could still investigate Babel’s use of
First, I looked at a lot of fixture tests. These are files meant to test if a particular Babel transform or plug-in works properly. They consist of two files:
actual.js (ES2015 code) and
expected.js. The goal of this test is to check if the output of
babel actual.js matches the content of
I noticed something about a particular transform :
babel-plugin-transform-es2015-parameters, more precisely about its handling of
This is unsafe. V8 will only be able to optimise the
concat function if the
arguments object has a length greater than 1. Otherwise, for example
concat(), the attempt to access the undefined
arguments will force V8 to rematerialize
arguments on the fly, preventing the whole function from being optimised.
Having no idea about Babel’s codebase and internals, it took me a whole weekend to come up with a first patch: #2833: Have es2015 rest transform safely use
arguments. It fixed some of the rest-transformed unsafe use of
arguments and it got merged after five weeks (which is way too long by the way, but I don’t blame anyone, I’m pretty sure it was an exceptional situation where someone said they would take care of this PR, then got busy, and in the meantime nobody saw the need to take over because someone was already in charge. No big deal).
At first I was pretty happy with this patch. The new
expected.js looked like this:
which was safe. Some basic benchmarks were showing a 4x speedup. The tests were green. I had learned a lot about how Babel works.
Until someone noticed the pattern I was using was a bit weird. In fact, the reason I initially chose this pattern was that I got it from here. While it makes sense to use it for default parameters handling (
ARGUMENTS.length <= INDEX || ARGUMENTS[INDEX] === undefined ? DEFAULT_VALUE : ARGUMENTS[INDEX];), it becomes overly complicated in the case where
I was fixing this pattern issue, replacing it with
ARGUMENTS.length <= INDEX ? undefined : ARGUMENTS[INDEX], when I noticed my previous patch was incomplete.
was still being converted to:
The transform was not taking into account the presence of a rest parameter when there were other parameters involved (
function (f, ...items)). After I fixed this issue, I had another one:
x = items was correctly transformed, but not
x = ...,
x.p = ... or
... = items || something. I had to generalise the patch to (safely) cover all possible occurrences of accessing a value from a rest argument.
I added a fixture test, reworked my patch and opened a new PR: Fixing T6818.
Hopefully, this part is done. I’ll try to find some other Crankshaft-related-JS-anti-patterns in what Babel generates.