GV

GeezerVison

The World Wide Web, restored to 800 pixels of glory.

Home|About|Gaming cabinet|Archives|How it works|Starter sites

On this site

  • Sign in
  • ActionsCI/CD and automation
  • CodespacesCloud development environments
  • All Features
  • Documentation
  • Training
  • Certifications
  • Build with Copilot
  • Customize Copilot
  • Integrations
  • Trending repos
  • GitHub Sponsors
  • Developer articles
  • Enterprises

This view

  • Original page
  • New address

GeezerVison is showing github.com — Tune FAR aggregation by amcasey · Pull Request #49581 · microsoft/TypeScript

Conversation

@amcasey

@amcasey Andrew Casey (amcasey) commented Jun 16, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

In making the work queue management more intelligible, we centralized the redundancy check at dequeue time. As a result, the queue tends to get very large (~1.6M items for SyntaxKind in this repo) and dequeuing via shift is too slow to do that many times. This change makes a few tweaks:

  1. Use Project identity for de-duping and only maintain a set of keys for loadAncestorProjectTree
  2. Attempt to filter prior to insertion
  3. Use splice if many consecutive work queue items will be discarded.

On my box, this cuts FAR for SyntaxKind in parser.ts from 38 minutes to 20 seconds (vs 26 seconds for 4.6) (we could do better, but effectively decided not to optimize this worst case scenario).

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
captain-yossarian, yukinotech, and jmopel reacted with rocket emoji
All reactions
  • 🚀 3 reactions
@amcasey
Tune FAR aggregation
58a4aad 
In making the work queue management more intelligible, we centralized the redundancy check at dequeue time.  As a result, the queue tends to get very large (~1.6M items for `SyntaxKind` in this repo) and dequeuing via `shift` is too slow to do that many times.  This change makes a few tweaks:

1. Use `Project` identity for de-duping and only maintain a set of keys for `loadAncestorProjectTree`
2. Attempt to filter prior to insertion
3. Use `splice` if many consecutive work queue items will be discarded.

On my box, this cuts FAR for `SyntaxKind` in parser.ts from 38 minutes to 20 seconds (we could do better, but effectively decided not to optimize this worst case scenario).
@amcasey
Andrew Casey (amcasey) requested review from Andrew Branch (andrewbranch) and Sheetal Nandi (sheetalkamat) June 16, 2022 20:00
@typescript-bot TypeScript Bot (typescript-bot) assigned Andrew Casey (amcasey) Jun 16, 2022
@typescript-bot TypeScript Bot (typescript-bot) added Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug labels Jun 16, 2022
Andrew Casey (amcasey) added 2 commits June 16, 2022 13:21
@amcasey
Add missing null-checks
f3a41fc
@amcasey
Fix lint errors
a12e29e
andrewbranch
Andrew Branch (andrewbranch) approved these changes Jun 16, 2022
View reviewed changes
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Jun 16, 2022
View reviewed changes
Comment thread src/server/session.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Jun 16, 2022
View reviewed changes
Comment thread src/server/session.ts Resolved

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) approved these changes Jun 16, 2022
View reviewed changes

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) left a comment

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I understand it; would be good to get another pair of eyes and get it in by today.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jun 16, 2022

Copy link
Copy Markdown
Member

Ah, Andrew Branch (@andrewbranch) already reviewed it. Still curious about the comments I left.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey
Andrew Casey (amcasey) force-pushed the FarQueue branch from 0388f07 to a12e29e Compare June 16, 2022 21:37
@amcasey
Remove redundant null checks and unreachable case
7f1e10c
@amcasey

Andrew Casey (amcasey) commented Jun 16, 2022

Copy link
Copy Markdown
Member Author

After a bunch of second-guessing myself, I'm pretty sure the null checks are now correct.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) approved these changes Jun 16, 2022
View reviewed changes
@amcasey
Andrew Casey (amcasey) merged commit 01ba3a4 into microsoft:main Jun 16, 2022
@amcasey
Andrew Casey (amcasey) deleted the FarQueue branch June 16, 2022 23:11
DanielRosenwasser
Daniel Rosenwasser (DanielRosenwasser) reviewed Jun 21, 2022
View reviewed changes
Comment thread src/server/session.ts
}

// NB: we may still skip if it's a project reference redirect
const { project, location } = queue.shift()!;

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Jun 21, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it make more sense long-term not to de-queue at all? Just iterate through and toss the array at the end?

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) Jun 21, 2022

Copy link
Copy Markdown
Member

There was a problem hiding this comment.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess not, then it would just get longer and longer.

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
All reactions
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jun 21, 2022

Copy link
Copy Markdown
Member

I realized that this is actually a pretty big 4.7 regression. If we have another patch release, it may be worth cherry-picking into 4.7.

TypeScript Bot (@typescript-bot) cherry-pick this to release-4.7
SlurpTheo reacted with eyes emoji
All reactions
  • 👀 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Daniel Rosenwasser (@DanielRosenwasser), I've started to run the task to cherry-pick this into release-4.7 on this PR at 7f1e10c. You can monitor the build here.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 21, 2022

Copy link
Copy Markdown
Contributor

Hey Daniel Rosenwasser (@DanielRosenwasser), I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.7 manually.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jun 21, 2022

Copy link
Copy Markdown
Member

Unfortunately looks like a pretty non-trivial cherry-pick. 😕
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented Jun 21, 2022

Copy link
Copy Markdown
Member Author

I realized that this is actually a pretty big 4.7 regression. If we have another patch release, it may be worth cherry-picking into 4.7.

TypeScript Bot (@typescript-bot) cherry-pick this to release-4.7

Daniel Rosenwasser (@DanielRosenwasser) Did you confirm that? I'm pretty sure this was regressed by the second part of my FAR cleanup, which was new in 4.8.

Would it make more sense long-term not to de-queue at all?

Yes, post-beta I plan to see whether I can eliminate the shift altogehter.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Contributor

Heya Andrew Casey (@amcasey), I've started to run the task to cherry-pick this into release-4.7 on this PR at 7f1e10c. You can monitor the build here.
DanielRosenwasser reacted with laugh emoji
All reactions
  • 😄 1 reaction

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@typescript-bot

TypeScript Bot (typescript-bot) commented Jun 21, 2022

Copy link
Copy Markdown
Contributor

Hey Andrew Casey (@amcasey), I couldn't open a PR with the cherry-pick. (You can check the log here). You may need to squash and pick this PR into release-4.7 manually.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@DanielRosenwasser

Daniel Rosenwasser (DanielRosenwasser) commented Jun 21, 2022 •
edited
Loading

Uh oh!

There was an error while loading. Please reload this page.

Copy link
Copy Markdown
Member

Daniel Rosenwasser (@DanielRosenwasser) Did you confirm that? I'm pretty sure this was regressed by the second part of my FAR cleanup, which was new in 4.8.

Okay, so just to be clear 4.7 is not exhibiting a 38 minute FAR search for SyntaxKind? 😅
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@andrewbranch

Andrew Branch (andrewbranch) commented Jun 21, 2022

Copy link
Copy Markdown
Member

38 minutes to 20 seconds

Oh, you know what, I initially read this as "38 seconds to 20 seconds" and I still thought that was pretty serious
amcasey and jmopel reacted with laugh emoji
All reactions
  • 😄 2 reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
@amcasey

Andrew Casey (amcasey) commented Jun 21, 2022

Copy link
Copy Markdown
Member Author

Daniel Rosenwasser (@DanielRosenwasser) Did you confirm that? I'm pretty sure this was regressed by the second part of my FAR cleanup, which was new in 4.8.

Okay, so just to be clear 4.7 is not exhibiting a 38 minute FAR search for SyntaxKind? 😅

I didn't specifically verify, but the shift is new in 4.8. If you want to confirm you can just FAR on SyntaxKind in parser.ts.
All reactions

Sorry, something went wrong.

Uh oh!

There was an error while loading. Please reload this page.
Andrew Casey (amcasey) added a commit to amcasey/TypeScript that referenced this pull request Jun 21, 2022
@amcasey
Add a simple queue implementation with better performance than `Array…
0c28bab 
….shift`

This lets us clean up the hack introduced in microsoft#49581
@amcasey Andrew Casey (amcasey) mentioned this pull request Jun 21, 2022
Add a simple queue implementation with better performance than Array.shift #49623
Merged
Andrew Casey (amcasey) added a commit that referenced this pull request Jun 24, 2022
@amcasey @Andarist
Add a simple queue implementation with better performance than `Array…
020ef41 
….shift` (#49623)

* Add a simple queue implementation with better performance than `Array.shift`

This lets us clean up the hack introduced in #49581

* Correct typo

Co-authored-by: Mateusz Burzyński <[email protected]>

Co-authored-by: Mateusz Burzyński <[email protected]>
@microsoft Microsoft (microsoft) locked as resolved and limited conversation to collaborators Oct 22, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.

Reviewers

@DanielRosenwasser Daniel Rosenwasser (DanielRosenwasser) DanielRosenwasser approved these changes
@andrewbranch Andrew Branch (andrewbranch) andrewbranch approved these changes
@sheetalkamat Sheetal Nandi (sheetalkamat) Awaiting requested review from sheetalkamat

Uh oh!

There was an error while loading. Please reload this page.

Assignees

@amcasey Andrew Casey (amcasey)

Labels

Author: Team For Uncommitted Bug PR for untriaged, rejected, closed or missing bug

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

Uh oh!

There was an error while loading. Please reload this page.

4 participants

@amcasey @DanielRosenwasser @typescript-bot @andrewbranch
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
You are visitor 01402503 — GeezerVison does not store the pages you look up.
Best viewed with Netscape Navigator 4.7 or Internet Explorer 6 at 800×600. No cookies required.