Bench/issue 2296 completion benchmark#4178
Open
kz357 wants to merge 2 commits into
Open
Conversation
LSP client spawns servers with bufsize=0, so stdout is a raw io.FileIO whose read(n) is a single syscall returning at most one pipe buffer, not n bytes. _rx_loop assumed read(length) returned whole body, so any response larger than pipe buffer came back truncated, failed to parse, and desynced the stream. Then the request would time out even though the server had already answered it fine. Go to Definition responses are small enough that this never happened, but I came across it while writing testcase. Reads the body in a loop instead
Adds --mode {definition,completion} to lsp_benchmark.py. In completion mode each run picks a member access on an imported module (eg pl.DataFrame after import polars as pl), truncates member to --prefix-len characters, and measures textDocument/completion at that point, so the server sees a partially-typed member access as it would while typing.
Server derives its filter prefix from the whole identifier under the cursor, so parking the cursor inside an existing complete token would filter on the full name instead of a short prefix. So truncation matters
Bc original member name is known, benchmark also checks that the server offered it back, which is what `valid` col reports in completion mode
Multi-package mode re-enters main() with a constructed argv so it picks up the new flags by forwarding them through
Follow-up to PR facebook#4148, per maintainer suggestion
Related: issue facebook#2296
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Follow up to PR #4148, per maintainer suggestion. Completions benchmark to catch future regressions on this path.
Fix short reads of large LSP responses-- client spawns servers with
bufsize=0so stdout is a rawio.FileIOwhoseread(n)is a single syscall returning at most one pipe buffer instead ofnbytes._rx_loopassumedread(length)returned full body, so any response longer than pipe buffer was truncated, failed to parse, and desynced the stream, and the request would time out even though the server had already answered it. Go to Definition responses are small so this never triggered, but a completion list is big enough that it would be problematic (verified through testing). Now it reads the body in a loop.ADd
--mode {definition,completion}-- in completion mode each run picks a member access on an imported module (egpl.DataFrameafterimport polars as pl), truncates member to--prefix-lenchars, and measurestextDocument/completionat that point, so the server sees a partially-typed member access (egpl.Din this case) as it would while typingServer derives its prefix filter from the whole identifier under the cursor, so putting the cursor inside an existing complete token would filter on the full name rather than a short prefix, and wouldn't reproduce the scenario in #2296, so truncation is necessary
Original member is known, so benchmark also verifies that the server offered it back, which is what
validcol reports in completion mode, so this measures correctness as well as latencyMulti-package mode re-enters
main()with a constructed argv, so it picks up new flags by forwarding them through so completion works in both single-repo and multi-package mode without a second benchmark loop.Related: issue #2296
Test Plan
Ran both modes against a small local repo (
import os/import jsonwith member accesses), using a pyrefly LSP on my local computer:--mode completion --runs 3 --seed 42gave ok=3/3, valid=3/3. Thevalidcolumn confirms pyrefly returnedgetcwdwhen given onlyos.g.valid=0/3there is pre-existing and the only symbols in that repo are stdlib, whose definitions resolve into bundled typeshed and so aren't on disk for the existing_looks_like_valid_locationcheckBefore the short-read fix, every completion request timed out at 10s despite server logging that it happened in ~0.2s, which is how the bug came up.
Mentioned latencies are from a debug build and not real but I think they're proof that the benchmark works :D