Expose `.index` on Position to internally track nodes location by tolmasky · Pull Request #14174 · babel/babel

tolmasky · 2022-01-18T16:38:37Z

Per the discussion in #14130, I am opening this PR to discuss the addition of an index property to the Position class. For context, we decided to use an indexes WeakMap in the above PR to avoid adding new members in a patch release, and figured we'd discuss whether to add this property for a minor release. In this current PR, I've kept it as an enumerable: false property, mainly to avoid having to generate all new fixtures, but I'd be happy to make it a normal property and do that too.

Also potentially relevant bug: #14173 - Although this bug can be done with either approach, it would certainly be more elegant to add more instances of loc.index than indexes.get(loc).

There may exist some performance argument for switching over to .index as well, but from what I've seen so far it doesn't seem to be too substantial.

Q	A
Fixed Issues?
Patch: Bug Fix?
Major: Breaking Change?
Minor: New Feature?	Yes
Tests Added + Pass?	Yes
Documentation PR Link
Any Dependency Changes?
License	MIT

nicolo-ribaudo

Yeah, let's expose .index. It significantly simplifies our internal implementation since we don't need to interact with the WeakMap anymore.

Also, I like that it consolidates all the location-related nodes data in .loc, rather than being scattered across three properties (well, we still have .start and .end, but @babel/parser consumers can now use just .loc).

nicolo-ribaudo · 2022-01-30T21:42:23Z

packages/babel-parser/src/util/location.js

    // this.index = index;
-    // Object.defineProperty(this, "index", { enumerable: false, value: index });
-    indexes.set(this, index);
+    Object.defineProperty(this, "index", { enumerable: false, value: index });


In this current PR, I've kept it as an enumerable: false property, mainly to avoid having to generate all new fixtures, but I'd be happy to make it a normal property and do that too.

Let's make it enumerable, even if it is a (one-time) annoying change that affects all our fixtures.

tolmasky · 2022-01-30T22:18:46Z

Cool, I just changed it to a normal property. This will make a bunch of tests fail of course, but I'll regenerate them and push that.

nicolo-ribaudo · 2022-01-30T22:20:54Z

You might have to regenerate them twice, the second time specifying the BABEL_8_BREAKING=true env flag.

tolmasky · 2022-01-30T23:51:28Z

Sounds good, it might end up touching every fixture because of that newline change from the last PR too.

existentialism

💯

joux3 · 2022-01-31T13:04:13Z

Just a small FYI: I wondered why prettier performance on their latest development branch had taken a hit. JS profiler showed the culprit to be the Position constructor. With the changes in this PR the performance problem is gone. I've attached parse times of 2000-line JS file when prettier runs 30 times in a row.

With indexes WeakMap

parse: 141.369ms
parse: 72.971ms
parse: 62.54ms
parse: 39.797ms
parse: 37.258ms
parse: 53.959ms
parse: 39.975ms
parse: 43.777ms
parse: 61.212ms
parse: 39.372ms
parse: 42.96ms
parse: 37.395ms
parse: 43.549ms
parse: 40.546ms
parse: 84.72ms
parse: 54.479ms
parse: 37.793ms
parse: 34.357ms
parse: 35.037ms
parse: 37.151ms
parse: 39.981ms
parse: 38.672ms
parse: 41.188ms
parse: 39.386ms
parse: 37.607ms
parse: 37.418ms
parse: 38.515ms
parse: 39.945ms
parse: 56.595ms
parse: 36.949ms

With index as a property on Position

parse: 125.497ms
parse: 54.491ms
parse: 38.092ms
parse: 30.726ms
parse: 29.803ms
parse: 27.034ms
parse: 32.897ms
parse: 33.431ms
parse: 28.893ms
parse: 27.017ms
parse: 25.962ms
parse: 24.177ms
parse: 24.763ms
parse: 22.585ms
parse: 26.872ms
parse: 22.765ms
parse: 24.774ms
parse: 30.861ms
parse: 35.862ms
parse: 25.584ms
parse: 25.947ms
parse: 25.213ms
parse: 21.999ms
parse: 22.546ms
parse: 20.826ms
parse: 21.315ms
parse: 21.283ms
parse: 27.481ms
parse: 22.744ms
parse: 21.553ms

tolmasky · 2022-01-31T14:17:46Z

Just a heads up so that it doesn't slip in with all the test changes, in order to get estree tests passing, I made it so that the estree plugin takes Positions and makes their index property non-enumerable. I specifically don't delete the property just in case it might be being used somewhere else (since we don't copy Positions when assigning them to nodes -- this is equivalent to the previous behavior and not anything new). The reason I didn't just update the estree tests instead, is because 1) if I understand correctly their point is to match the spec that we don't control, and 2) even when I do do that, we still fail the tests the compare our results with espree (in parseAndAssertSame). Here are the relevant changes: f8b85e4

tolmasky · 2022-01-31T14:19:41Z

Hit comment a bit too soon, wanted to end the above thought by saying that I'm happy to change the strategy with estree if someone proposes something else, and wanted to also explain that this issue didn't come up until I made index a public (and not undefined) property, as when it is either "private" or undefined, it doesn't get compared in the diffing algorithm.

nicolo-ribaudo · 2022-01-31T15:56:00Z

Yeah I noticed that you made it non-enumerable in ESTree; I don't particularly care about what it is since technically we shouldn't have a property (because ESTree doesn't have it), but removing it from the AST would require a full traversal. I'm fine with just "partially hiding" it as you did.

@tolmasky

…oach. Reviewed by @tolmasky.

@tolmasky

…property. Reviewed by @tolmasky.

@tolmasky

Reviewed by @tolmasky.

@tolmasky

Reviewed by @tolmasky.

@tolmasky

Reviewed by @tolmasky.

@tolmasky

Reviewed by @tolmasky.

@tolmasky

Reviewed by @tolmasky.

babel-bot · 2022-02-02T13:20:26Z

Build successful! You can test your changes in the REPL here: https://babeljs.io/repl/build/51042/

tolmasky mentioned this pull request Jan 21, 2022

Improve errors location tracking #14130

Merged

nicolo-ribaudo added pkg: parser PR: New Feature 🚀 A type of pull request used for our changelog categories labels Jan 30, 2022

nicolo-ribaudo approved these changes Jan 30, 2022

View reviewed changes

tolmasky force-pushed the use-index-property-insead-of-indexes-weakmap branch from ad339d4 to 88cb9a3 Compare January 30, 2022 22:17

tolmasky force-pushed the use-index-property-insead-of-indexes-weakmap branch 2 times, most recently from 4b6d25e to 259a9c2 Compare January 31, 2022 03:25

existentialism approved these changes Jan 31, 2022

View reviewed changes

nicolo-ribaudo added this to the v7.17.0 milestone Jan 31, 2022

tolmasky force-pushed the use-index-property-insead-of-indexes-weakmap branch from bb26a68 to c7133ff Compare January 31, 2022 14:16

JLHwung approved these changes Jan 31, 2022

View reviewed changes

tolmasky added 7 commits February 2, 2022 14:13

Use an index property on Position instead of the indexes WeakMap appr…

40475e0

…oach. Reviewed by @tolmasky.

Make index a normal property of Position instead of a non-enumerable …

4faff93

…property. Reviewed by @tolmasky.

Make index non-enumerable when using the estree plugin.

c3c12a8

Reviewed by @tolmasky.

Update tests.

d717c52

Reviewed by @tolmasky.

Update this test.

efa6ab6

Reviewed by @tolmasky.

Update more tests.

78d98fa

Reviewed by @tolmasky.

Update Babel 8 tests.

f2f4618

Reviewed by @tolmasky.

nicolo-ribaudo force-pushed the use-index-property-insead-of-indexes-weakmap branch from c7133ff to d193a18 Compare February 2, 2022 13:17

nicolo-ribaudo changed the title ~~Use an index property on Position instead of the indexes WeakMap approach~~ Expose .index on Position to internally track nodes locations Feb 2, 2022

Update after rebase

d776456

nicolo-ribaudo force-pushed the use-index-property-insead-of-indexes-weakmap branch from d193a18 to d776456 Compare February 2, 2022 13:21

nicolo-ribaudo changed the title ~~Expose .index on Position to internally track nodes locations~~ Expose .index on Position to internally track nodes location Feb 2, 2022

nicolo-ribaudo merged commit 97a8bcb into babel:main Feb 2, 2022

This was referenced Feb 15, 2022

Performance issues with new versions of @babel/parser netlify/zip-it-and-ship-it#968

Closed

[Bug]: Position includes index = undefined #14182

Closed

github-actions bot added the outdated A closed issue/PR that is archived due to age. Recommended to make a new issue label May 5, 2022

github-actions bot locked as resolved and limited conversation to collaborators May 5, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Expose `.index` on Position to internally track nodes location#14174

Expose `.index` on Position to internally track nodes location#14174
nicolo-ribaudo merged 8 commits intobabel:mainfrom
tolmasky:use-index-property-insead-of-indexes-weakmap

tolmasky commented Jan 18, 2022 •

edited by gitpod-io bot

Loading

Uh oh!

nicolo-ribaudo left a comment •

edited

Loading

Uh oh!

nicolo-ribaudo Jan 30, 2022

Uh oh!

tolmasky commented Jan 30, 2022

Uh oh!

nicolo-ribaudo commented Jan 30, 2022

Uh oh!

tolmasky commented Jan 30, 2022

Uh oh!

existentialism left a comment

Uh oh!

joux3 commented Jan 31, 2022

Uh oh!

tolmasky commented Jan 31, 2022

Uh oh!

tolmasky commented Jan 31, 2022

Uh oh!

nicolo-ribaudo commented Jan 31, 2022

Uh oh!

babel-bot commented Feb 2, 2022 •

edited

Loading

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

Uh oh!

Conversation

tolmasky commented Jan 18, 2022 • edited by gitpod-io bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nicolo-ribaudo left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nicolo-ribaudo Jan 30, 2022

Choose a reason for hiding this comment

Uh oh!

tolmasky commented Jan 30, 2022

Uh oh!

nicolo-ribaudo commented Jan 30, 2022

Uh oh!

tolmasky commented Jan 30, 2022

Uh oh!

existentialism left a comment

Choose a reason for hiding this comment

Uh oh!

joux3 commented Jan 31, 2022

Uh oh!

tolmasky commented Jan 31, 2022

Uh oh!

tolmasky commented Jan 31, 2022

Uh oh!

nicolo-ribaudo commented Jan 31, 2022

Uh oh!

babel-bot commented Feb 2, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

tolmasky commented Jan 18, 2022 •

edited by gitpod-io bot

Loading

nicolo-ribaudo left a comment •

edited

Loading

babel-bot commented Feb 2, 2022 •

edited

Loading