Skip to content

Improve the performance of requesting completions within a massive array literal #40953

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

Merged
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions src/compiler/core.ts
Original file line number Diff line number Diff line change
Expand Up @@ -1195,7 +1195,7 @@ namespace ts {
* @param keyComparer A callback used to compare two keys in a sorted array.
* @param offset An offset into `array` at which to start the search.
*/
export function binarySearchKey<T, U>(array: readonly T[], key: U, keySelector: (v: T) => U, keyComparer: Comparer<U>, offset?: number): number {
export function binarySearchKey<T, U>(array: readonly T[], key: U, keySelector: (v: T, i: number) => U, keyComparer: Comparer<U>, offset?: number): number {
if (!some(array)) {
return -1;
}
Expand All @@ -1204,7 +1204,7 @@ namespace ts {
let high = array.length - 1;
while (low <= high) {
const middle = low + ((high - low) >> 1);
const midKey = keySelector(array[middle]);
const midKey = keySelector(array[middle], middle);
switch (keyComparer(midKey, key)) {
case Comparison.LessThan:
low = middle + 1;
Expand Down
12 changes: 11 additions & 1 deletion src/services/utilities.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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, _) => {
Copy link
Member

Choose a reason for hiding this comment

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

A comparer that ignores one argument is definitely not more readable than what you started with.

Copy link
Member Author

@weswigham weswigham Oct 6, 2020

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

I think a comment would be helpful here.

if (position < children[middle].end) {
// first element whose end position is greater than the input position
if (!children[middle - 1] || position >= children[middle - 1].end) {
return Comparison.EqualTo;
}
return Comparison.GreaterThan;
}
return Comparison.LessThan;
});
if (i >= 0 && children[i]) {
const child = children[i];
// Note that the span of a node's tokens is [node.getStart(...), node.end).
// Given that `position < child.end` and child has constituent tokens, we distinguish these cases:
Expand Down
19 changes: 19 additions & 0 deletions tests/cases/fourslash/excessivelyLargeArrayLiteralCompletions.ts

Large diffs are not rendered by default.