Tune FAR aggregation #49581
Tune FAR aggregation #49581
Conversation
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).
typescript-bot
added
Author: Team
For Uncommitted Bug
|
Ah, @andrewbranch already reviewed it. Still curious about the comments I left. |
|
After a bunch of second-guessing myself, I'm pretty sure the null checks are now correct. |
| queue.splice(0, skipCount); | ||
| } | ||
|
|
||
| // NB: we may still skip if it's a project reference redirect | ||
| const { project, location } = queue.shift()!; |
There was a problem hiding this comment.
Would it make more sense long-term not to de-queue at all? Just iterate through and toss the array at the end?
There was a problem hiding this comment.
I guess not, then it would just get longer and longer.
|
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 cherry-pick this to release-4.7 |
|
Heya @DanielRosenwasser, I've started to run the task to cherry-pick this into |
|
Hey @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. |
|
Unfortunately looks like a pretty non-trivial cherry-pick. 😕 |
@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.
Yes, post-beta I plan to see whether I can eliminate the |
|
Hey @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. |
Okay, so just to be clear 4.7 is not exhibiting a 38 minute FAR search for |
Oh, you know what, I initially read this as "38 seconds to 20 seconds" and I still thought that was pretty serious |
I didn't specifically verify, but the |
….shift` This lets us clean up the hack introduced in microsoft#49581
….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]>
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
SyntaxKindin this repo) and dequeuing viashiftis too slow to do that many times. This change makes a few tweaks:Projectidentity for de-duping and only maintain a set of keys forloadAncestorProjectTreespliceif many consecutive work queue items will be discarded.On my box, this cuts FAR for
SyntaxKindin 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).