-
Notifications
You must be signed in to change notification settings - Fork 5.2k
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
perf(lsp): Call serverRequest
via V8 instead of via executeScript
#23409
Conversation
60f45fd
to
6e1786a
Compare
Re-posting for visibility: maybe we could have a giant enum that you send over the channel and perform direct v8 serialization in the TSC thread? |
904381d
to
b5c4107
Compare
cli/lsp/tsc.rs
Outdated
) -> Result<(&'static str, Option<v8::Local<'s, v8::Value>>), AnyError> { | ||
let args = match self { | ||
TscRequest::ProjectChanged(args) => { | ||
("$projectChanged", Some(serde_v8::to_v8(scope, args)?)) |
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.
Suggestion: consider giving these a numeric ID that you then map back in 99_compiler_main.js
- it's gonna be faster than having to serialize these strings, but it's gonna be a bit wonky to keep track of them. Might be worth the trade-off 🤞
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.
We could do something like this to make it more robust to maintain a numeric mapping:
- Use
enum_derive
or something like it to derive an iterator for all variants on the Rust side. - Keep an enum in JS with the numeric mappings in some sort of object form (ie: easily converted to a string with the names and values):
enum TscRequest {
requestA = 0,
requestB,
...
}
- Have a callback that requests the full contents of that enum, only used for a unit test in the next step
- Have a unit test on the CLI/LSP side that compares whether the JS enum and Rust enums is up-to-date or not.
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.
Looks good to me, I would still encourage to try and make these reqs have numeric IDs instead of sending strings over.
b5c4107
to
5952f33
Compare
5952f33
to
ed8d8d3
Compare
…or TS result upon cancellation (#23645) Fixes #23643. We weren't catching the cancellation exception thrown by TSC on the JS side, so the rust side was catching this exception and then attempting to print out the exception via `toString`. That last bit resulted in a cryptic `[object Object]` showing up in the logs like so: ``` Error during TS request "getCompletionEntryDetails": [object Object] ``` I'm not 100% sure how we weren't seeing this in the past. My guess is that #23409 and the subsequent PR to improve the exception catching and logging surfaced this, but I'm still not quite clear on it. My initial fix here returned `null` to rust when a server request was cancelled, but this resulted in a deserialization error when we attempted to deserialize that into the expected response type. So now, as soon as the request's cancellation token signals we'll stop waiting for a response and return an error (which will get swallowed as the LSP request is being cancelled). I was a bit surprised to find that [this branch](https://github.com/nathanwhit/deno/blob/0c671c9792ac706c1ecd60f88efdc5eb8e941917/cli/lsp/tsc.rs#L1093) actually executes sometimes, I believe due to the fact that aborting a future may not [immediately stop its execution](https://docs.rs/futures/latest/futures/stream/struct.AbortHandle.html#method.abort).
Doesn't have a noticeable perf impact from my benchmarking, but theoretically should be better.