Don't compile and transform all scripts for each VU if archive #1040
Conversation
This was a regression introduced with 3506ee1. The core issue lies with a difference in how we run archive files and not archived scripts. If the script is not archived we run it once in order to gather all the files that will be required and to calculate the options among other things. This is not required for archives as it was done when the archive was created, so this step was skipped and the fist time the files were executed was when a VU was created. This should've lead to executing and loading each file for each VU previously as well if not for the fact that all the code is in map. That map is first generated in the original `bundle` of source codes and than copied to a new bundle when a VU is created. Or more accurately it is now copied, previously just a reference to the map was copied. All of this together lead to the fact that the first VU that gets started will populate the map and all VUs after that would've used it. After it is copied this didn't happen. Obviosly this will have rece condition of VUs where initialized concurently, but they aren't. But PR#1007 also initializes VUs concurrently which would've lead to ... probably some panics ;(. The current fix is the minimalist somewhat concurrent and future proof fix that is to just run the script once when we load it from the archive in order to just cache all the compilation and transformation. Maybe in the future we can use that now calculated values although I have the sneaking suspicion that there will be dragons once we start testing with things like enviromental variables and complex configs, not to speak of precedences of configurations.
Codecov Report
@@ Coverage Diff @@
## master #1040 +/- ##
==========================================
- Coverage 72.67% 72.64% -0.04%
==========================================
Files 133 133
Lines 9846 9848 +2
==========================================
- Hits 7156 7154 -2
- Misses 2274 2276 +2
- Partials 416 418 +2
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Hmm this should work even with #1007, but don't very much like this fix... architecturally? We probably shouldn't have to transform and instantiate the archives until we need the VUs, but as we discussed, that might be complicated to do... But even so, using instantiate() for the purpose of only babel-transforming the files somehow doesn't feel very clean and I'm wondering if it won't have other unintended consequences 😕
There was a problem hiding this comment.
LGTM, couldn't think of any unintended consequences from this change, beyond a very minor but unnecessary performance hit, so it's probably not worth it to dig deeper now. @mstoykov, can you add a new performance / refactoring / low prio issue to properly fix this sometime in the future before you merge it?
This was a regression introduced with 3506ee1. The core issue lies with
a difference in how we run archive files and not archived scripts.
If the script is not archived we run it once in order to gather all the
files that will be required and to calculate the options among other
things. This is not required for archives as it was done when the
archive was created, so this step was skipped and the fist time the
files were executed was when a VU was created. This should've lead to
executing and loading each file for each VU previously as well if not
for the fact that all the code is in map. That map is first generated in
the original
bundleof source codes and than copied to a new bundlewhen a VU is created. Or more accurately it is now copied, previously
just a reference to the map was copied.
All of this together lead to the fact that the first VU that gets
started will populate the map and all VUs after that would've used it.
After it is copied this didn't happen.
Obviosly this will have rece condition of VUs where initialized
concurently, but they aren't. But PR#1007 also initializes VUs
concurrently which would've lead to ... probably some panics ;(.
The current fix is the minimalist somewhat concurrent and future proof
fix that is to just run the script once when we load it from the archive
in order to just cache all the compilation and transformation.
Maybe in the future we can use that now calculated values although I
have the sneaking suspicion that there will be dragons once we start
testing with things like enviromental variables and complex configs, not
to speak of precedences of configurations.