feat: Add locations option to parser#16935
Conversation
|
Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/61133 |
| export class Position { | ||
| // If `declare` is used here, the `jquery` benchmark will drop by 10%. :) | ||
| line: number; | ||
| column: number; | ||
| index: number; | ||
| declare index?: number; |
There was a problem hiding this comment.
Ugh, this is super weird, I tried for a long time before I found it was related to it.🤦♂️
| if (!locDataCache || locDataCache.length < (this.length + 1) * 2) { | ||
| locDataCache = new Uint32Array((this.length + 1) * 2); | ||
| } |
There was a problem hiding this comment.
Performance drops by about 10% when there is no cache.
| this.raise( | ||
| Errors.MissingEqInAssignment, | ||
| this.optionFlags & OptionFlags.Locations | ||
| ? node.left.loc.end | ||
| : node.left, | ||
| ); |
There was a problem hiding this comment.
Here is one of the ones that were skipped during testing.
| ? this.optionFlags & OptionFlags.Locations | ||
| ? // @ts-expect-error node.key has been guarded | ||
| node.key.loc.end | ||
| : // @ts-expect-error node.key has been guarded | ||
| node.key |
There was a problem hiding this comment.
Here is one of the ones that were skipped during testing.
99ad61b to
58efcf9
Compare
JLHwung
left a comment
There was a problem hiding this comment.
I like the option flags bit packs. Can you extract that refactor into a small PR so that we don't have to wait for the next minor release?
58efcf9 to
4cbe9a0
Compare
| declare loc: SourceLocation; | ||
| } | ||
|
|
||
| let locDataCache: Uint32Array; |
There was a problem hiding this comment.
Is the location data cache shared between different parser inputs?
There was a problem hiding this comment.
Yes, actually this is just sharing a Uint32Array instance to avoid the time-consuming creation of a new one, the data inside is not shared.
There was a problem hiding this comment.
IIRC the data inside is correct if and only if for every getLoc call, there is a setLoc call preparing the location data for the exact index. If the getLoc call is issued before setLoc, it will read 1) uninitialized locDataCache cells or 2) location data used by previous inputs. This is prone to errors because of the implicit constraint here that if one should throw an error at the location, the location must have been prepared by setLoc, and there will be no immediate errors if it happens that the previous input gives the same location data.
I would suggest we zero-filling the locDataCache in the parse() entry and check if they are zero when getLoc is called, if they are zero, we should throw an internal error reminding contributors to manually add the setLoc call.
There was a problem hiding this comment.
I added it in the non-release version.
Because it affects performance and the user may still be able to use it normally even if the location information is wrong.
| // `startLoc` and `endLoc` properties. | ||
|
|
||
| export class Position { | ||
| // If `declare` is used here, the `jquery` benchmark will drop by 10%. :) |
There was a problem hiding this comment.
Did you mean if declare is used for line and column? Since the index property is declared.
There was a problem hiding this comment.
Yes, if I use declare for line and column, the performance will decrease.🤦♂️
b9e72c6 to
07a7af5
Compare
3050870 to
5897bd6
Compare
|
commit: |
9985853 to
98b96a2
Compare
b198d53 to
0084a35
Compare
0084a35 to
e1c1e96
Compare
e1c1e96 to
1eb0516
Compare
5b24f3b to
17506a3
Compare
17506a3 to
92e717d
Compare
|
To clarify, how does this PR affect performance in the default case of using |
|
This PR should have no impact at present. Ideally, we can switch the default usage of |
|
@liuxingbaoyu Could you rebase? |
92e717d to
f4f696c
Compare
f4f696c to
c63904b
Compare
|
Could you open a docs PR? 🙏 |
Fixes #1, Fixes #2This allows Babel to be faster in certain use cases and fixes the performance regression in the
estreeplugin.I tried many, many approaches and this was the one with the least performance impact and the fewest changes.
0~5% performance loss in the benchmark, ~10% without caching.
before
after
(no-loc seems to reduce performance for small files, which is strange)
estree