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

Investigate core.js performance impacts on VU initialization and memory usage #1036

Closed
na-- opened this issue May 31, 2019 · 8 comments
Closed
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 js-compat performance

Comments

@na--
Copy link
Member

na-- commented May 31, 2019

Prompted by this StackOverflow question, I briefly investigated why VUs take so much memory. I found out that the major culprit for the memory usage (of simple scripts) is the core.js library we include in every VU. We need it as a polyfil, because goja doesn't implement big chunks of the ECMAScript 6+ specs, but if I remove these three lines: https://github.com/loadimpact/k6/blob/2999a1e8e6657d88853f8d65fe195226173645e2/js/bundle.go#L246-L248
And run a simple script with 2000 VUs, on my machine k6 initializes those 2000 VUs in under 2 seconds and takes just under 300 MB of RAM, instead of the ~10000MB mentioned in the StackOverflow question (that I also confirmed locally with k6 0.24.0).

Something that might help with this memory usage is for us to avoid importing the whole core.js library, maybe something like this: https://babeljs.io/docs/en/babel-preset-env#usebuiltins-usage-experimental

Or, have a CLI flag/environment variable that tells k6 "I am a JS professional, I know what I'm doing". Basically, a mode that disables any babel and core.js work from k6 and relies that the user has passed their script through whatever babel/browserify/webpack/rollup/core.js/typescript/some-new-js-thing conversion process they desire and passes the ES5.1-compliant result to k6 for execution.

@na-- na-- added performance js-compat evaluation needed proposal needs to be validated or tested before fully implementing it in k6 labels May 31, 2019
@matlockx
Copy link

matlockx commented May 31, 2019

quick and dirty would be to init it once in js/lib/lib.go:

var coreJs *goja.Program = nil
func initCoreJs() {
        coreJs = goja.MustCompile(
                "core-js/shim.min.js",
                rice.MustFindBox("core-js").MustString("shim.min.js"),
                true,
        )
}
func GetCoreJS() *goja.Program {
        return coreJs
}

and set it in js/lib/rice-box.go at the end:

func init() {
...

     initCoreJs()
}

if i do not understand something wrong

@na--
Copy link
Member Author

na-- commented Jun 3, 2019

Edit: disregard, totally misread the comment above 🤦‍♂️
Unfortunately, something like that probably won't work, for at least 2 reasons:
1. It will be a very major breaking change - if we require that users explicitly opt into core.js inclusion, a lot of existing scripts will break.
2. That opt-in probably can't happen in a JavaScript function - it will likely need to be an import in the global script scope, which you can't get to from inside of a function.

And in general, we probably can't opt in or out of a JS polyfill from inside of a JS script, since that polyfill may have been required to evaluate that script in the first place. That's why I only mentioned a CLI flag and/or environment variable as the viable places for such a config toggle in the original issue.

@matlockx
Copy link

matlockx commented Jun 3, 2019

i'm a bit confused, what i was pointing out is just a simple go solution which compiles the program only once and returns only, if requested, the pointer to that program. so you would get the same pointer no matter how often you call the GetCoreJS func.
Thought it's probably also a nice feature to opt out from command line my proposed change should not break the existing behavior,
i looked a bit deeper into the code and i do not see some mutating code on the program so i would suggest that it's save to use the same pointer in different vus. also: the current implementation is using the user program in a same way: compile once and reuse the program

@na--
Copy link
Member Author

na-- commented Jun 3, 2019

Ah, sorry, totally disregard my previous message, my sleep-addled brain completely misread half of your proposal...

@na--
Copy link
Member Author

na-- commented Jun 3, 2019

After second (more caffeinated) look at your proposal, you are absolutely right! I never noticed that we're parsing core.js anew for every VU 🤦‍♂️ Sorry again for my previous stupid response... 😊

I'll submit a PR with a fix shortly, though I'll probably use sync.Once instead of putting the core.js parsing into an init() function. That way, we'll still do it only one time, but if we don't need it (for k6 subcommands that don't require it or for when/if we have that CLI flag disabling it in the future), we won't parse it even that one time.

@matlockx
Copy link

matlockx commented Jun 3, 2019

sounds good. the proposal with the flag for babel seems also nice ;)

@matlockx
Copy link

matlockx commented Jun 4, 2019

i guess this can be closed now, or do you wait until the next release? btw, are you planning to do one in the near future?

@na--
Copy link
Member Author

na-- commented Jun 13, 2019

Yes, thank you again for pointing out this simple fix! 😄
Regarding a new release - yes, we're planning a new official k6 release in the next 2 weeks, to release #1038 (this improvement) as well as the fixes in #1040 and #1047, among others.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
evaluation needed proposal needs to be validated or tested before fully implementing it in k6 js-compat performance
Projects
None yet
Development

No branches or pull requests

2 participants