Improve the performance of requesting completions within a massive array literal #40953
Conversation
typescript-bot
added
Author: Team
For Milestone Bug
|
@typescript-bot perf test this - this shouldn't affect perf tests here at all, since this is only used in a LS codepath, which the perf tests don't really do; as such I expect no changes. @minestarks I assume the crawler is collecting some kind of perf metrics (?) - should we be monitoring it for a before/after type picture with this change? While this definitely helps pathological cases like this one, it does incur some small overhead in more typical cases where nodes have only one or two children; it'd be nice to know if I needed to special case the small-N scenario to still be a linear scan, but I can only really tell if it is necessary with some data. |
|
Heya @weswigham, I've started to run the perf test suite on this PR at da57f98. You can monitor the build here. Update: The results are in! |
|
@weswigham Here they are:Comparison Report - master..40953
System
Hosts
Scenarios
|
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
|
Does the fuzzer even have coverage for completion? |
Nah, that condition doesn't work - it hangs forever, same as current search, and fails some tests besides (just tried it again, just in case I missed something). As I said in my comment - the |
|
Ehh, I still rephrased the code to use |
| @@ -1173,7 +1173,17 @@ namespace ts { | |||
| } | |||
|
|
|||
| const children = n.getChildren(sourceFile); | |||
| for (let i = 0; i < children.length; i++) { | |||
| const i = binarySearchKey(children, position, (_, i) => i, (middle, _) => { | |||
There was a problem hiding this comment.
A comparer that ignores one argument is definitely not more readable than what you started with.
There was a problem hiding this comment.
So... that we call it a "comparer" is itself kinda a mistake on our part. Yes, you can pass a comparer and it does what you might want, but we reliably pass the second argument to binarySearchKey as the second argument to the "comparer" - it never changes. I could choose to not ignore it, but there's no reason to - it's the same constant position value I close over.
There was a problem hiding this comment.
So it's more of a "selector" callback - it just needs to return if you "select the pivot element", "select the left group", or "select the right group". The Comparison values are just mapped to that.
There was a problem hiding this comment.
I think a comment would be helpful here.
By binary searching for the first child whose
endposition is greater than the input position, rather than scanning them all in order (since tokens and nodes are generally sorted by both theirposandendin lists ofchildren). This takes the runtime of the example given in the linked issue (which is the test verbatim) down from "will this ever end, I've waited at least 5 minutes" to a somewhat more reasonable 11s.Fixes #40100