-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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 LSP benchmark mimicking the one on quick-lint-js #13365
Conversation
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'm not sure why you are seeing 128ms instead of 200ms minimum, looking at the code is from the time the change is processed it should be at least 200ms before we start an update_diagnostics
...
This still isn't right because I'm using
|
Well, it will be 200ms + tsc diagnostic request time + maybe blocking time if there is some other request in tsc. The whole purpose of the debounce was to allow other "high priority" tsc requests to get in front of the diagnostic request, but requests are processed in the order they are received: Lines 100 to 111 in eda6e58
Given the test case though I can't specifically think of another request that might be going on, but it could be. I did check on a reasonably sized project and I am getting:
So getting 287ms for a tsc diagnostics publish on a large file is not unreasonable or unexpected. The first one being < 200ms I believe is actually another side effect all together of a design limitation of the test harness. We tried and tried to find a non-blocking way to "poll" for messages, but haven't, so any unread unsolicited messages will be read and returned, but if you try to poll for a message that isn't there, it will just hang. So you are likely reading something that was previously sent. |
I've updated the benchmark to wait until we get the first "deno-lint" diagnostic (as described in the source field of the diagnostics. After this lands I'll send a PR to quick-lint-js to change their benchmark. It's surprising but each iteration is still quite a bit longer than 200ms.
|
There are other factors at play that try to keep it from becoming a "blocking" thing, but as I mentioned above, I think it can be refactored so that there is no debouncing of lint and "deno" diagnostics, we only debounce the TypeScript ones, and then we would get a "real" picture of how long it takes to issue lint diagnostics as well as provide a more responsive experience for users. |
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.
LGTM noting a couple nits
See #13022
https://github.com/quick-lint/quick-lint-js/blob/35207e6616267c6c81be63f47ce97ec2452d60df/benchmark/benchmark-lsp/lsp-benchmarks.cpp#L223-L268
Currently I'm getting something like 128ms per iteration (which seems slower than the 200ms expected) so not sure if there is something broken.