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

Run imports once per VU #975

Merged
merged 4 commits into from
Mar 28, 2019
Merged

Run imports once per VU #975

merged 4 commits into from
Mar 28, 2019

Conversation

mstoykov
Copy link
Contributor

Previously scripts would've been ran for each time they were imported
this is both slower and not how it works in most(all?) interpreters
so we are changing the behavious to be more compliant.

Just like in c3d3fa5 , but now we clear
the cached exports when we copy the programs when making new VUs.

This is needed because otherwise:

  1. We share exports between VUs so there are race conditions
  2. The exports from the initial compilation without context are used
    (when not running from archive) and this leads to not being able to call
    functions that need context inside imported scripts.

Closes #659 and fixes #969

@mstoykov mstoykov requested a review from na-- March 22, 2019 15:44
@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #975 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #975      +/-   ##
=========================================
+ Coverage   72.08%   72.1%   +0.01%     
=========================================
  Files         131     131              
  Lines        9602    9607       +5     
=========================================
+ Hits         6922    6927       +5     
  Misses       2267    2267              
  Partials      413     413
Impacted Files Coverage Δ
js/initcontext.go 100% <100%> (ø) ⬆️
js/console.go 94.87% <0%> (-0.13%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 651146b...0d7c20f. Read the comment docs.

@codecov
Copy link

codecov bot commented Mar 22, 2019

Codecov Report

Merging #975 into master will increase coverage by 0.01%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff            @@
##           master    #975      +/-   ##
=========================================
+ Coverage   72.08%   72.1%   +0.01%     
=========================================
  Files         131     131              
  Lines        9602    9608       +6     
=========================================
+ Hits         6922    6928       +6     
  Misses       2267    2267              
  Partials      413     413
Impacted Files Coverage Δ
js/initcontext.go 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 3b1161c...0086a84. Read the comment docs.

Previously scripts would've been ran for each time they were imported
this is both slower and not how it works in most(all?) interpreters
so we are changing the behavious to be more compliant.

Just like in c3d3fa5 , but now we clear
the cached exports when we copy the programs when making new VUs.

This is needed because otherwise:
1. We share exports between VUs so there are race conditions
2. The exports from the initial compilation without context are used
(when not running from archive) and this leads to not being able to call
functions that need context inside imported scripts.

Closes #659 and fixes #969
This works by prepopulating what a given file(module) will export before
actually evaluationg the code whatsoever.

This way if it requires something that requires the original
module it will get the exports object as it is at that time. And because
of some magic (probably in babel) we also get that the exports are
bindings[1]. So even if something is not exported at the time of the
import, it will still be populated later when the original file finish
loading.

[1]: http://2ality.com/2015/07/es6-module-exports.html#why-export-bindings

Fixes #502
Copy link
Member

@na-- na-- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM, besides the minor copy-paste issues

js/module_loading_test.go Show resolved Hide resolved
js/module_loading_test.go Outdated Show resolved Hide resolved
@na--
Copy link
Member

na-- commented Mar 27, 2019

btw I'm guessing that the module resolution function suffers from some of the same file path issues that open() did. It probably should be in a separate issue/PR, but just mentioning it here as something work looking into

@mstoykov
Copy link
Contributor Author

Oh ... definitely although here it seems it was very deliberate.
My opinion on the matter is that we should probably do as browsers have supposedly agreed to do it. In our case this will mean requiring remote imports to have https:// and everything that doesn't start with / ./ ../ to be considered a k6 module :)
But definitely a different PR

@mstoykov mstoykov merged commit 3506ee1 into master Mar 28, 2019
@na-- na-- deleted the loadModulesOnce branch April 23, 2019 13:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Init context creates multiple instances of global variable ES6 modules instantiation works incorrectly
2 participants