Adds default targets for babel configuration

[ncordon]

Solves a Javascript perfomance problem. Closes #4322.

Kudos to @angrykoala for helping me debug and solve the issue.

[ncordon]

Hey @ericvergnaud I see you maintain the Javascript target in the README. Any thoughts on this?

It makes Javascript parsers ~30 times faster than what they are nowadays.

[ericvergnaud]

@ncordon thanks for this, and sorry that I didn't have time lately to review this PR.
tbh I very much doubt that this changes makes parsers 30 x faster, maybe you could show how the babelized code differs to explain such a difference ?
also I'd want to know what this change breaks in terms of compatibility ?

[ncordon]

tbh I very much doubt that this changes makes parsers 30 x faster, maybe you could show how the babelized code differs to explain such a difference ?

In the babel docs I linked in the issue they recommend setting the browser targets to avoid big bundles, setting it to recommended defaults cuts the transpiled code down to a third of the original size. The problem with not setting it at all is that will make the code compatible with every single browser released (all the code is transpiled down to be compatible to ES5).

I left a reproduction case in the issue and tried already with a different grammar which is the internal one we have at Neo4j, not the one that is published in the official antlr grammars. And the x30 speed up was consistent. I can try with more grammars if needed? 😄

also I'd want to know what this change breaks in terms of compatibility ?

According to babel docs: Because of this, Babel's behavior is different than browserslist: it does not use the defaults query when there are no targets are found in your Babel or browserslist config(s). If you want to use the defaults query, you will need to explicitly pass it as a target. So that means if my understanding is correct, according to browserlist that would remove support for the dead browsers (without official support or updates for 24 months) listed there: IE 11, IE_Mob 11, BlackBerry 10, BlackBerry 7, Samsung 4, OperaMobile 12.1 and all versions of Baidu.

[ericvergnaud]

Thanks for the rationale. Reducing the file size would definitely reduce the download time, but since your benchmark runs on node, I don't think it's involved in the boost that you observe.
In my experience, the JS runtime is roughly 8 times slower than the Java one. A 30 x boost goes against everything we know about the relative performance of these 2 languages...
That's why I'm asking for a babelized code extract that could explain your observation.

[angrykoala]

Hi @ericvergnaud
You are completely right about the relative performance of the two languages, that's why it seems strange such a time difference, which may hint at a bug somewhere.

I followed the steps in issue #4322, running the benchmark provided after executing npm run build with the default .babelrc:

{
"presets": ["@babel/preset-env"]
}

Which yields an execution time of ~178.96 ms

With the same exact setup, just adding the option targets: defaults, so the .babelrc looks like this:

{
  "presets": ["@babel/preset-env"],
  "targets": "defaults"
}

After running npm run build, the benchmark now takes ~6.68ms

I do not know why this may happen, and I understand that it is not an expected behaviour, but it is clear by the benchmark that, with this grammar in node 18.16, the configuration of Babel is affecting the execution performance by ~27x (not download nor build time)

I'm happy to provide the babelize code or any further information if you are not able to reproduce the problem, I'm also happy to make any changes to the benchmark if there is something that we may be doing wrong.

Using the runtime code directly without Babel also runs at ~7ms, which is why we suspect Babel.

In my experience, the JS runtime is roughly 8 times slower than the Java one. A 30 x boost goes against everything we know about the relative performance of these 2 languages...

Could you please point me to the benchmarks you are using to arrive at this conclusion? Maybe this will help pinpoint where the problem is or if our setup is incorrect.

[ericvergnaud]

The 8x difference is not from a formal benchmark, rather from my experience when using antlr with simple to complex grammars, during which I measured performance as part of other observations (recursion depth, # of calls to closure...).
Unfortunately I don't have time to reproduce your findings, hence why I'm asking that you provide babelized code extracts that could explain your benchmark.
The problem with accepting your PR is that I suspect it would impact many users, so we need to be able to not only describe the benefit but also explain it. Then they can make an informed decision on whether or not to migrate - the JS ecosystem is a total mess and I get a lot of noise from users because we don't support build tool X with language version Y in framework Z...

[angrykoala]

Hi @ericvergnaud
Thank you for the quick response 🙂
This PR is a possible fix to a measured performance problem. This fix is just a proposal for fixing #4322, if there is a better fix for that, then we are happy to do that instead, I'll comment my thoughts on how this could be done better below, but as it is now, this is the easiest solution we could find to this issue.

Regarding the reason, as @ncordon pointed out, according to Babel docs and its best recommendations, this is currently trying to bundle to the highest compatibility possible, instead of the versions defined in the project, which makes me suspect of polyfills or other compatibility techniques that affect performance severely on modern js runtimes.

I understand the issues with how messy the ecosystem is and the risk of breaking things. In my experience (and here I'm opinionated and you may know better) a library that is intended to be used by other projects should not be bundled with something like Webpack by default. Node.js projects do not need any bundling at all (which, as we measured, also fixes the issue) and front-end projects will have their own bundling systems that should take care of this (and also, should avoid collisions with yours). Taking other libraries, such as joi as an example show that the approach is to use minified/bundled code for the browser, but keep the plain .js files for Node.js.

I'm not sure why this bundling is being done in the first place tbh (I'm not that experienced nor do I know the project enough), my best guess is to fix the whole commonjs vs es6 import mess in the ecosystem. I appreciate that this is messy and other approaches may be more work, but a 30x performance for a parser is quite a big deal (even if this only happens for some grammars) IMO.

Unfortunately I don't have time to reproduce your findings, hence why I'm asking that you provide babelized code extracts that could explain your benchmark.

I'm sorry to hear that you don't have time to reproduce the issue.
As requested, I'm attaching the Babel versions for which the file benchmark.js shows 30x difference. In case the diff between these may help. But I hope you understand that I cannot feasibly debug a transpiled version of a codebase I don't know to pinpoint the problem. Much less so, when the root issue is probably caused by the transpilation process, not the codebase itself
antlr_js_dist.zip

[ncordon]

For what it's worth I've made a fork of @KvanTTT's benchmarks, repo here, and run them only for Java and Javascript.

The relative loss of perfomance between 4.8 and 4.13 is noticeable:

For 4.8:

| Runtime                                 |  Mean [ns] |      Ratio |
| ---                                     |        --- |        --- |
| JavaStandard_NotLeftRecursion           |     171426 |       1.00 |
| JavaScriptStandard_NotLeftRecursion     |     678276 |       3.96 |
| JavaStandard_LeftRecursion              |     211838 |       1.24 |
| JavaScriptStandard_LeftRecursion        |     736566 |       4.30 |

For 4.13:

| Runtime                                 |  Mean [ns] |      Ratio |
| ---                                     |        --- |        --- |
| JavaStandard_NotLeftRecursion           |     166219 |       1.00 |
| JavaScriptStandard_NotLeftRecursion     |   38756208 |     233.16 |
| JavaStandard_LeftRecursion              |     257626 |       1.55 |
| JavaScriptStandard_LeftRecursion        |   35087463 |     211.09 |

[ericvergnaud]

Thanks that's useful
FWIW I've posted a survey on google groups, see whether some people have good reasons to stick to ES5

[ncordon]

Thanks a lot @ericvergnaud 🤗

[ncordon]

Any updates on this @ericvergnaud? I see the Google group discussion thread did not get any answers. Maybe a thread could be opened in the more active GitHub Discussions, just to be sure?

[ericvergnaud]

@ncordon they've had their chance. merging...
@parrt blessed