Speeding up tc39 tests through using a pool of Babels #1839
Conversation
mstoykov
force-pushed
the
speedingUpTests
branch
from
9c6f290
to
a1a210a
Compare
Codecov Report
@@ Coverage Diff @@
## master #1839 +/- ##
==========================================
- Coverage 71.60% 70.09% -1.52%
==========================================
Files 179 178 -1
Lines 13928 13936 +8
==========================================
- Hits 9973 9768 -205
- Misses 3323 3530 +207
- Partials 632 638 +6
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Is the performance hit for regular tests because the babel object is short-lived? According to the sync.Pool docs:
On the other hand, a free list maintained as part of a short-lived object is not a suitable use for a Pool, since the overhead does not amortize well in that scenario. It is more efficient to have such objects implement their own free list.
In any case, I vote 👎 for this as is, since the main test suite taking ~15m will be painful every time, whereas the current ~11m runtime of the TC39 suite is perfectly fine, especially since it runs only on js/* changes.
Codecov Report
@@ Coverage Diff @@
## master #1839 +/- ##
==========================================
- Coverage 72.11% 72.10% -0.02%
==========================================
Files 180 180
Lines 14243 14256 +13
==========================================
+ Hits 10272 10279 +7
- Misses 3346 3351 +5
- Partials 625 626 +1
Flags with carried forward coverage won't be shown. Click here to find out more.
Continue to review full report at Codecov.
|
js/compiler/compiler.go
Outdated
| if err != nil { // TODO cache this? | ||
| return | ||
| } |
There was a problem hiding this comment.
Why would we need to cache it? Actually, is the err check and return here needed at all?
There was a problem hiding this comment.
in practice ... it should never be an error, I am almost fine with panicking as if babel doesn't compile ... well there is nothing we can do anymore ;)
There was a problem hiding this comment.
I'm not sure you answered my question 😅 If Babel doesn't compile then this would only fail once, so what's the point of caching it and of the TODO? And since we check this err a few lines below this, I don't see the point of this additional check and return from this lambda.
There was a problem hiding this comment.
OKay .. the whole if is ... kind of not needed as this whole thing is inside once.Do.
But the idea behind the caching is that k6 will execute this once.Do .. once and only the first call will actually set the err for it's execution, all other executions will block on the once.Do until finished and will return a nil globalBabelCode
There was a problem hiding this comment.
Ah, I see, so as a way to prevent blocking other goroutines? I would at least explain that in the TODO, as it's unclear why right now. Though given this is such an unlikely edge case I would just remove the check and TODO.
mstoykov
changed the title
|
I will squash to 3 commits:
when we agree it looks okay to merge ;) |
|
I don't think I will be doing anything else for now apart from rewriting commits once I get some 👍 (edit: I had a commit not pushed 🤦 ) Here is a list of all the newly skipped tests with an explanation on how it was generated (in the comment) |
There was a problem hiding this comment.
This LGTM, but I'm wondering if the Compiler pooling could've been done in the tc39 package instead. I'm always wary of changes that are only useful for tests, especially in a core package like compiler.
Also nitpick, but I'd prefer deleting lines instead of commenting them out, unless they're temporary and you're sure we might need them back.
And I'd really appreciate a before/after run time comparison in the description :)
The words are `async`, `yield`, `generator` and `Generator`. This does remove nearly 1k previously ran test that were totally unsupported, some of which "passed" as they expected it will not run.
A lot of tc39 test need to compile and transpile the code of the test before they can execute it. Babel *does* take significant amount of time to transpile even small programs and having only one means that on machines with 8 cores most of the time is being spent waiting for babel. This can be used in other places at a later point - `k6/http` and some `js` package libs are also mostly babel transpile dominated. This speeds up the running of the test by around 4x on my machine. An
mstoykov
force-pushed
the
speedingUpTests
branch
from
a6076f5
to
a05a128
Compare
|
I deleted the commented lines and did unexport one of the methods that doesn't need to be exported anymore, along rebasing stuff to three commits. I did the compilerpool in tc39 but it will actually be useful in both some Also .... literally all the current caching in that package even now is for testing as the normal k6 can only compile/transpile one file at a time and could've just initialized one babel of it's own ... but in tests it makes a big difference we don't initialize babel for each time we need it (the goja compiler is a lot less of a problem ;) ) |
There was a problem hiding this comment.
LGTM. Looking at the tests from other PRs, this speeds up the TC39 tests by 2 minutes (8 vs 6m), and the others don't seem to be impacted and have similar run times. So the description should at least be updated to remove "all other tests suffer".
Nice work! 👏
|
OH ... I keep forgetting this. |
PoC pool of babels so transformation is less bottlenecked on them
this speeds up tc39 tests
It also has the nice benefit of dropping the babels once they are no
longer needed if you just run k6.
But a different approach is probably going to be needed as the js package
(for example) with -race takes upwards of 3x as much time.