Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use NodeFlags to detect nodes inside with statements instead of climbing ancestors #17721

Merged
3 commits merged into from
Oct 23, 2017

Conversation

ghost
Copy link

@ghost ghost commented Aug 10, 2017

Previously, every time we needed to know whether a node was in a with statement, we would climb all of its ancestor nodes looking for a with statement. Since there usually wasn't one, this would take a while.

Now we just add a NodeFlags for this during parsing. This is similar to the optimization done to detect whether we're in JSDoc. (#17176 (comment))

This appears to give a 1% average improvement in performance in the checker, without any major slowdown in the parser.

Monaco - node (v8.2.1, x64)

Project Baseline Current Delta Best Worst
Memory used 356,983k (± 0.01%) 356,966k (± 0.01%) -17k (- 0.00%) 356,905k 357,082k
Parse Time 2.12s (± 0.69%) 2.12s (± 0.50%) -0.00s (- 0.09%) 2.10s 2.15s
Bind Time 0.82s (± 0.63%) 0.82s (± 0.75%) +0.00s (+ 0.24%) 0.81s 0.83s
Check Time 4.30s (± 0.59%) 4.17s (± 0.49%) -0.13s (- 3.03%) 4.12s 4.20s
Emit Time 2.80s (± 1.40%) 2.76s (± 0.96%) -0.03s (- 1.11%) 2.73s 2.85s
Total Time 10.03s (± 0.74%) 9.87s (± 0.41%) -0.16s (- 1.63%) 9.78s 9.94s

Monaco - tsc (x86)

Project Baseline Current Delta Best Worst
Parse Time 1.48s (± 1.10%) 1.49s (± 0.59%) +0.01s (+ 0.40%) 1.47s 1.51s
Bind Time 0.57s (± 2.08%) 0.58s (± 2.56%) +0.01s (+ 1.40%) 0.55s 0.62s
Check Time 4.04s (± 1.97%) 3.96s (± 1.13%) -0.08s (- 2.01%) 3.86s 4.03s
Emit Time 7.65s (± 3.77%) 7.59s (± 0.99%) -0.06s (- 0.81%) 7.52s 7.86s
Total Time 13.75s (± 2.26%) 13.62s (± 0.72%) -0.13s (- 0.96%) 13.45s 13.87s

TFS - node (v8.2.1, x64)

Project Baseline Current Delta Best Worst
Memory used 309,625k (± 0.01%) 309,627k (± 0.01%) +3k (+ 0.00%) 309,542k 309,666k
Parse Time 1.54s (± 2.26%) 1.55s (± 2.43%) +0.00s (+ 0.13%) 1.44s 1.58s
Bind Time 0.70s (± 5.85%) 0.70s (± 5.63%) -0.00s (- 0.71%) 0.66s 0.81s
Check Time 3.68s (± 0.42%) 3.68s (± 1.73%) +0.00s (+ 0.05%) 3.61s 3.93s
Emit Time 2.45s (± 0.34%) 2.46s (± 0.55%) +0.01s (+ 0.24%) 2.44s 2.49s
Total Time 8.38s (± 0.29%) 8.38s (± 0.77%) +0.00s (+ 0.05%) 8.30s 8.62s

TFS - tsc (x86)

Project Baseline Current Delta Best Worst
Parse Time 1.07s (± 1.78%) 1.08s (± 1.52%) +0.00s (+ 0.47%) 1.05s 1.12s
Bind Time 0.57s (± 4.34%) 0.58s (± 2.75%) +0.01s (+ 1.22%) 0.53s 0.61s
Check Time 3.29s (± 1.79%) 3.26s (± 1.71%) -0.03s (- 0.82%) 3.16s 3.41s
Emit Time 4.53s (± 1.59%) 4.47s (± 0.87%) -0.06s (- 1.32%) 4.39s 4.57s
Total Time 9.46s (± 0.90%) 9.39s (± 0.70%) -0.07s (- 0.78%) 9.26s 9.53s

@ghost ghost requested a review from rbuckton August 10, 2017 15:17
@ghost
Copy link
Author

ghost commented Aug 29, 2017

@rbuckton Any comments?

@ghost
Copy link
Author

ghost commented Sep 22, 2017

@rbuckton Good to go?

@ghost
Copy link
Author

ghost commented Oct 22, 2017

@rbuckton Bump

@ghost ghost merged commit 159a0a2 into master Oct 23, 2017
@ghost ghost deleted the inWithStatement branch October 23, 2017 20:38
@microsoft microsoft locked and limited conversation to collaborators Jun 14, 2018
This pull request was closed.
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants