-
Notifications
You must be signed in to change notification settings - Fork 29.4k
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
Add telemtry for how long it takes to parse files with tree-sitter #213565
Conversation
42c4efa
to
7f0678a
Compare
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
CI failure |
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.
Nice work!
There are still some race conditions that I think should be fixed before merging this though.
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
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.
@hediet, thanks for the review! I tried to capture all of your feedback.
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
} | ||
|
||
override dispose() { | ||
super.dispose(); | ||
this._treeSitterTree?.dispose(); | ||
this._unregisterModelListeners(); | ||
this._languageSessionDisposables.dispose(); |
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 is not needed, as the this._register
call takes care of that :)
The nice thing about _register
is that you can see at the creation location in the source that it is eventually going to be disposed.
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.
🤦♀️
src/vs/editor/browser/services/treeSitter/treeSitterParserService.ts
Outdated
Show resolved
Hide resolved
} | ||
return this._onDidChangeContent(model, e); | ||
}).catch((e) => { | ||
this._logService.error('Error parsing tree-sitter tree', e); |
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 onUnexpectedError
might also be good here, because then you'd see it in telemetry.
try { | ||
tree = this.parser.parse((index: number, position?: Parser.Point) => this._parseCallback(model, index), this.tree); | ||
} catch (e) { | ||
// parsing can fail when the timeout is reached, will resume upon next loop |
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.
Can you check the error type and re-throw the error for non-expected errors?
…icrosoft#213565) * Make space for tree sitter * Add the tree sitter wasm file * Very naive tree-sitter syntax highlighting for html, with a layer breaker * Update tree when content changes * WIP for making abstract tokens class * Handle theme changes * Replace entire text model value with parse callback * Perf improvements * Add tree-sitter-typescript * Add typescript + better initial parsing * Refactor into tree parsing service and fix flaw in parse callback * Remove things that aren't the parser service * Add yielding * Remove changes that aren't required for PR * Remove more file changes * Reduce yield to 50 ms * Fix incremental parsing * Try update node-abi * Revert "Try update node-abi" This reverts commit df28801. * Update text buffer chunk api * fix build * Remove tree-sitter dependency * Adopt new, as yet unpublished, `@vscode/tree-sitter-wasm` package * Use published `@vscode/tree-sitter-wasm` package * Break `TreeSitterTree` and `TreeSitterParserService` into better pieces and: - document the order of editor changes - use service injection where `TextModel` is constructed * Fix tests * Remove unneeded import * Fix missing tree-sitter-wasm in web and remote * Make package.jsons match * Add @vscode/tree-sitter-wasm to web loader config * Try using importAMDNodeModule * PR feedback * Add race condition test for changing language while loading language * Use same timeout * Queue content changes * Remove override dispose * Move queue into TreeSitterTree --------- Co-authored-by: Peng Lyu <penn.lv@gmail.com>
Part of #210475