-
Notifications
You must be signed in to change notification settings - Fork 646
Increase overall performance by speeding up GoDocumentSymbolProvider #1766
Conversation
Decreases time to get symbols by 1-2 orders of magnitiude in large files This in turn seems to speed up formatting time. Still WIP, will look for other performance pitfalls.
I have been using this for over a week, didn't run into any issues. |
@OneOfOne were you experiencing perf issues before? |
@FiloSottile can you try this vsix and see if the performance improves? |
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.
Run some checks to compare numbers between existing code, this code and the intermediate solution of creating the buffer only once and then passing to convertToCodeSymbols
src/avlTree.ts
Outdated
case -1: return BalanceState.SLIGHTLY_UNBALANCED_RIGHT; | ||
case 1: return BalanceState.SLIGHTLY_UNBALANCED_LEFT; | ||
case 2: return BalanceState.UNBALANCED_LEFT; | ||
default: return BalanceState.BALANCED; |
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.
This would return balance when height difference is more than 2 which wouldn't happen in our case as we re-balance after adding every node, but for correctness sake, unbalanced left/right should be returned
src/avlTree.ts
Outdated
/** | ||
* Gets the value of a node within the tree with a specific key. | ||
* @param key The key being searched for. | ||
* @param root The root of the tree to search in. |
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.
missing comment for closest
src/avlTree.ts
Outdated
} | ||
|
||
/** | ||
* Gets the value of a node within the tree with a specific key. |
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.
Update the overall comment as well as the one for @return to convey that the "node" is returned not the "value of the node".
Same foes for the _get
function
src/goOutline.ts
Outdated
decls: GoOutlineDeclaration[], | ||
symbols: vscode.SymbolInformation[], | ||
containerName: string, | ||
byteOffsetToDocumentOffset: (offset: number) => number): void { |
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.
rename the parameter from offset
to byteoffset
here and every other place we are talking about converting byte offset to offset
src/goOutline.ts
Outdated
} | ||
}); | ||
} | ||
|
||
public provideDocumentSymbols(document: vscode.TextDocument, token: vscode.CancellationToken): Thenable<vscode.SymbolInformation[]> { | ||
let t0 = Date.now(); |
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.
unused variable
This removes massive copy overhead from recreating Buffers of the entire document for every symbol in the file. Also reduces the quadratic time approach to converting from byte-offsets to character-offsets to log-linear time.
Hopefully closes #1668.