codegen performance: use trim instead of lodash/trimEnd#5255
codegen performance: use trim instead of lodash/trimEnd#5255loganfsmyth merged 4 commits intobabel:masterfrom jwbay:codegen-perf
Conversation
|
@jwbay, thanks for your PR! By analyzing the history of the files in this pull request, we identified @loganfsmyth, @davidaurelio and @jridgewell to be potential reviewers. |
| _trimEnd(str: string): string { | ||
| let trimTarget = str.length; | ||
| const whitespace = /\s/; | ||
| while (trimTarget > 0 && str[trimTarget - 1].match(whitespace)) { |
There was a problem hiding this comment.
whitespace.test(str[trimTarget - 1])
There was a problem hiding this comment.
Wasn't aware of the perf difference. Done, thanks!
| const map = this._map; | ||
| const result = { | ||
| code: trimEnd(this._buf.join("")), | ||
| code: this._trimEnd(this._buf.join("")), |
There was a problem hiding this comment.
Is trimming actually necessary?
There was a problem hiding this comment.
As a new contributor I'm not really equipped to answer this question. Trailing whitespace doesn't seem important, but I guess there could be misbehaving plugins that introduce large amounts of it.
There was a problem hiding this comment.
To throw out a guess, the generator injects lots of newlines trying to make formatting match, so this probably keeps the end-of-file formatting the same even through the node type at the end of the file may differ.
There was a problem hiding this comment.
^ That's what I assumed it was as well
Codecov Report@@ Coverage Diff @@
## master #5255 +/- ##
=======================================
Coverage 89.22% 89.22%
=======================================
Files 203 203
Lines 9885 9885
Branches 2663 2663
=======================================
Hits 8820 8820
Misses 1065 1065
Continue to review full report at Codecov.
|
|
oh, or there's https://www.npmjs.com/package/trim-right which already has this optimization (from none other than @kittens). |
|
Wow a good find @jridgewell! #2008 |
|
For reference: this was originally changed from trim-right to lodash in #3500; should probably add a comment so it doesn't get switched back again |
|
And maybe we can try to get this improvement into lodash? :) |
| this._queue.unshift([str, line, column, identifierName, filename]); | ||
| } | ||
|
|
||
| _trimEnd(str: string): string { |
There was a problem hiding this comment.
Could we throw a comment on here so it is clear why we do this rather than Lodash? Over time it'll be easy to lose info about which issue introduced this, and why.
|
Nice find!
|
|
Thanks for doing this @jwbay. I've been super busy with work, so your contribution is very appreciated! /cc @DmitriiAbramov since Jest is waiting on this fix for a new release. |
|
Ah, checking a bit more: This was a case where I had previously optimized it but thought it was a bit overkill. But now there's a nice scenario, so 🤘. Out of curiosity @jwbay could you test it with lodash.trimright (the v3 version of _.trimEnd). It used things like |
|
Ran the above with |
|
@jdalton: Note that the new code is considerably slower when there are a large amounts of spaces at the end. We could probably speed it up further by doing a |
|
Thanks @existentialism!
Would you mind sanity checking the built-in
That's what Lodash v3 _.trimRight does. |
|
I ran the tests on node 6.9.4. With |
|
@existentialism Great! Ya, I remember it being pretty speedy. 27.201798 ms
0.010351 ms
0.002709 ms
0.001111 ms
0.000863 ms
0.000719 ms
0.00078 ms
0.00075 ms
0.000703 ms
0.000747 ms
0.000757 ms
0.00074 ms
0.000728 ms
0.000742 ms
0.000726 ms
0.000743 ms
0.000796 ms
0.000711 ms
0.000716 ms
0.000722 ms
Update: 0.083213 ms
0.009484 ms
0.002044 ms
0.001321 ms
0.00106 ms
0.000942 ms
0.00086 ms
0.000897 ms
0.001407 ms
0.001589 ms
0.001432 ms
0.001167 ms
0.0015 ms
0.001465 ms
0.00106 ms
0.000834 ms
0.000871 ms
0.00085 ms
0.00081 ms
0.000839 msMaybe just go with the built-in all up. |
|
I switched to the built-in and left a comment in case it needs to be switched to something else in the future. |
|
Only It looks like two output format tests fail if we just use |
|
Ok, switched to trim. Here's the run on node 6: |
|
Hey @jwbay! It looks like one or more of your builds have failed. I've copied the relevant info below to save you some time. |
lodash/trimEnd executes a regex against potentially massive strings which can freeze node. instead find out how much we need to trim off character by character, then slice it off all at once
| @@ -1,4 +1,2 @@ | |||
| #!/usr/bin/env node | |||
|
|
|||
|
|
|||
loganfsmyth
left a comment
There was a problem hiding this comment.
@jridgewell is totally right, we can't use .trim() because the sourcemaps will point to the wrong lines.
|
Should we go back to local impl. or use https://www.npmjs.com/package/trim-right? |
|
Why don't we just use |
Because |
|
@jdalton It'd break the Web REPL on IE, at least according to MDN. |
|
@loganfsmyth isn't that a core-js concern? |
|
Babel relies on |
|
What is |
|
As a library, Babel doesn't make any assumptions about non-ES5 method availability and it relies on trim-right was linked above in #5255 (comment) as something that has this optimization. We can also just wait if you plan to optimize this in Lodash. I have no string feelings about how we fix it. |
What's
Ya, it looks like it's comparable: 0.180566 ms
0.018806 ms
0.026337 ms
0.002897 ms
0.002399 ms
0.002713 ms
0.002148 ms
0.002398 ms
0.00205 ms
0.002236 ms
0.001994 ms
0.002223 ms
0.003143 ms
0.002006 ms
0.001956 ms
0.001909 ms
0.001959 ms
0.001976 ms
0.219092 ms
0.002705 ms
Lodash is into its v5 development. v4 updates are restricted to critical bugs. So this optimization won't come until v5 is released (some time away) |
Wait, IE8 doesn't support indexed string access? I'd say IE8 is beyond Babel's supported versions. I actually don't think we have an official number, but using Since we can't wait on Lodash v5, and we can get acceptable performance from |
Right-ish. It's wonky.
Yiss! It'd be nice to have Babel's support clearly outlined though to make things,
Does Babel support Nashorn? Are you testing Nashorn?
Yap. Lodash 5 will actually rely on |
|
Nashorn or IE aren't officially supported according to compiler-environment-support.md I agree with @loganfsmyth, this is not worth breaking them. |
Thanks!
Got it. Don't break enviros that are untested with unknown (un)supported states. 👌 😸 |
|
We are going to drop transform-runtime in 7.0 btw |
lodash/trimEnd executes a regex against potentially massive strings which can freeze node. Instead find out how much we need to trim off character by character, then slice it off all at once
Fixes #5081
Benchmark:
Result, first with lodash, then with local implementation: