Skip to content
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

New option to disable the automatic babel transpiling and core.js inclusion #1049

Closed
na-- opened this issue Jun 13, 2019 · 2 comments · Fixed by #1206
Closed

New option to disable the automatic babel transpiling and core.js inclusion #1049

na-- opened this issue Jun 13, 2019 · 2 comments · Fixed by #1206

Comments

@na--
Copy link
Member

na-- commented Jun 13, 2019

As seen in #1036 (comment), core.js is responsible for the vast majority of VU memory usage and initialization time for simple scripts, even after the fixes from #1038.

Also, currently babel transformation happens on the "do it if it fails without it": https://github.com/loadimpact/k6/blob/2a2fe2cc665e0d2b818c4f3ca7ce4fc9a5821294/js/compiler/compiler.go#L131-L146

I think this is wrong for a few different reasons:

  • it's unpredictable and confusing - some files of a project may be transpiled, others may not be
  • it's probably slower - likely, the vast majority of files will fail the direct goja parsing, so they will need to be transpiled, so the initial parsing is a waste of effort
  • there are probably corner cases where goja could parse a file at the surface level and not return an error, but when it later tries executing it, it would fail

So the proposal is that by default k6 would always transpile files with babel and include core.js, but we'll have a flag to completely disable that behavior for users that don't need these things or for power users that have their own script bundling/transformation pipeline.

@na-- na-- added enhancement docs performance js-compat evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Jun 13, 2019
@na-- na-- added this to the v0.26.0 milestone Aug 27, 2019
@na--
Copy link
Member Author

na-- commented Sep 19, 2019

After some discussion regarding this, @sniku proposed that instead of a combined --no-js-magic or individual --no-babel and --no-corejs bool flags, we should instead have a single string flag that would be used to specify the execution/compatibility mode k6 should use. I like this approach very much, since it's much more flexible and extensible than individual options, and in the future it could allow us to add even more customization options... It could even be used to support JS-adjacent scripts like TypeScript (#422), whereas the current --type flag would probably be used for completely different things like Go (#751) or lua.

Not sure what the best name for something like that is, but for lack of a better option, I'll go with --compatibility-mode over --execution-mode for now, so that it isn't confused with the completely different execution options from #1007, but better name suggestions are very much welcome! For now, we can start with just 2 possible options:

  • The default one should be the current state - use babel when needed (i.e. first try to run each script/import without babel, and if that fails, transpile it) and always include core-js in each VU. The lazy use of babel is useful both for preserving backwards compatibility and for not wasting time on already ES5.1 compatible imports (usually browserified libraries). Not sure what the actual option name for this should be though, since I can't encapsulate the current complicated logic in a few characters... sometimes-babel-always-corejs? 😄 Or maybe we should just call it default?
  • es5.1 would be a compatibility mode that doesn't use babel or core-js, it would use only goja. This would be useful to save a lot of memory (Investigate core.js performance impacts on VU initialization and memory usage #1036 (comment)), and as a target for webpack/rollup/browserify/whatever-js-tool pipelines that advanced JS users would like to have for their code.
  • If my concerns about the unpredictability of the current nondeterministic babel transformation are valid, we can add a mode that always executes babel, though we should probably wait to see if there's actually need for that...

@na-- na-- added high prio refactor and removed evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels Sep 19, 2019
@imiric imiric self-assigned this Sep 24, 2019
imiric pushed a commit to imiric/k6 that referenced this issue Sep 25, 2019
imiric pushed a commit to imiric/k6 that referenced this issue Sep 25, 2019
imiric pushed a commit to imiric/k6 that referenced this issue Sep 25, 2019
@imiric
Copy link
Contributor

imiric commented Oct 1, 2019

As discussed last week, I tried to implement the --compatibility-mode option without the config refactor, but find it problematic.

In summary, the --compatibility-mode or $K6_COMPATIBILITY_MODE value needs to be passed down to three places:

The main problem is that all configuration newRunner() and thus everything below it receives is lib.RuntimeOptions (Which IMHO is a confusing name for an os.Environ() wrapper. I understand it's using Goja semantics, but it's still part of configuration, so I removed it in my config API refactor.).
So the JS compiler only has environment variables and the script itself as configuration source, which is a problem if using a --compatibility-mode CLI flag. ;) I could hack around this, force the CLI flag into lib.RuntimeOptions or pass an additional compatibilityMode argument everywhere, modifying all signatures in the process, but I think both approaches are inferior to slightly fixing the config API by ensuring configuration is parsed and aggregated once in cmd and passed as config.Config to all sub-packages.

@na-- @mstoykov @cuonglm Let me know if you have a better idea of handling this so that we can avoid or postpone the config refactor.

imiric pushed a commit that referenced this issue Oct 17, 2019
imiric pushed a commit that referenced this issue Oct 17, 2019
imiric pushed a commit that referenced this issue Oct 25, 2019
Partially implements #1049.

This required a substantial refactor of `compiler.go`, mostly in order
to be able to test a compiler with different compatibility mode values
during the same test run, which was problematic since `compiler.Compiler`
was a singleton. So the solution is to instead make a new `babel` struct
the singleton, which encapsulates everything needed for Babel to run,
while allowing several `Compiler` instances to exist. In the author's
humble opinion, this has the added benefit of being a cleaner design
(which was further experimentally expanded in 4e52c78).
imiric pushed a commit that referenced this issue Oct 25, 2019
imiric pushed a commit that referenced this issue Oct 25, 2019
Partially implements #1049.

This required a substantial refactor of `compiler.go`, mostly in order
to be able to test a compiler with different compatibility mode values
during the same test run, which was problematic since `compiler.Compiler`
was a singleton. So the solution is to instead make a new `babel` struct
the singleton, which encapsulates everything needed for Babel to run,
while allowing several `Compiler` instances to exist. In the author's
humble opinion, this has the added benefit of being a cleaner design
(which was further experimentally expanded in 4e52c78).
imiric pushed a commit that referenced this issue Oct 25, 2019
imiric pushed a commit that referenced this issue Nov 4, 2019
Partially implements #1049.

This required a substantial refactor of `compiler.go`, mostly in order
to be able to test a compiler with different compatibility mode values
during the same test run, which was problematic since `compiler.Compiler`
was a singleton. So the solution is to instead make a new `babel` struct
the singleton, which encapsulates everything needed for Babel to run,
while allowing several `Compiler` instances to exist. In the author's
humble opinion, this has the added benefit of being a cleaner design
(which was further experimentally expanded in 4e52c78).
imiric pushed a commit that referenced this issue Nov 4, 2019
imiric pushed a commit that referenced this issue Nov 6, 2019
Partially implements #1049.

This required a substantial refactor of `compiler.go`, mostly in order
to be able to test a compiler with different compatibility mode values
during the same test run, which was problematic since `compiler.Compiler`
was a singleton. So the solution is to instead make a new `babel` struct
the singleton, which encapsulates everything needed for Babel to run,
while allowing several `Compiler` instances to exist. In the author's
humble opinion, this has the added benefit of being a cleaner design
(which was further experimentally expanded in 4e52c78).
imiric pushed a commit that referenced this issue Nov 6, 2019
imiric pushed a commit that referenced this issue Nov 11, 2019
Partially implements #1049.

This required a substantial refactor of `compiler.go`, mostly in order
to be able to test a compiler with different compatibility mode values
during the same test run, which was problematic since `compiler.Compiler`
was a singleton. So the solution is to instead make a new `babel` struct
the singleton, which encapsulates everything needed for Babel to run,
while allowing several `Compiler` instances to exist. In the author's
humble opinion, this has the added benefit of being a cleaner design
(which was further experimentally expanded in 4e52c78).
imiric pushed a commit that referenced this issue Nov 11, 2019
imiric pushed a commit that referenced this issue Nov 12, 2019
Partially implements #1049.

This required a substantial refactor of `compiler.go`, mostly in order
to be able to test a compiler with different compatibility mode values
during the same test run, which was problematic since `compiler.Compiler`
was a singleton. So the solution is to instead make a new `babel` struct
the singleton, which encapsulates everything needed for Babel to run,
while allowing several `Compiler` instances to exist. In the author's
humble opinion, this has the added benefit of being a cleaner design
(which was further experimentally expanded in 4e52c78).
imiric pushed a commit that referenced this issue Nov 12, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants