Commit the solve behind a type query on an unopened file - #4698
Commit the solve behind a type query on an unopened file#4698samwgoldman wants to merge 1 commit into
Conversation
This comment has been minimized.
This comment has been minimized.
ff2b077 to
73c0d64
Compare
This comment has been minimized.
This comment has been minimized.
kinto0
left a comment
There was a problem hiding this comment.
great change, thank you!!
would love to see tests on the following:
-
Repeated unopened-file query reuse
Query the same unopened file twice and verify the second query performs no solve work. This is the primary purpose of the commit. -
Deferred didChange visibility
Arrange didChange(v2) → type query → later mutation, causing validation of v2 to be deferred. Verify the query returns the type from v2 rather than stale shared state. -
Non-committable fallback
Hold the recheck lock, issue a type query, and verify its solved transaction is saved. Release the lock and verify the next query reuses and commits it. -
Intervening resolveImport
After saving a query transaction, call resolveImport, then issue another query. This would expose the transaction-loss issue from my review. -
Invalidation after caching
Query an unopened file, change it, advance the snapshot, and verify the next query recomputes rather than returning the cached old type. -
Explicit queue-draining case
Use a type conversion known to lazily enqueue additional work and verify the transaction commits successfully. Existing type-shape tests may exercise this accidentally, but none assert the invariant. -
Other unopened query kinds
Only getComputedType tests unopened files. getDeclaredType and getExpectedType now use the same new path but lack equivalent coverage.
| /// which is a little longer but avoids resolving a handle against state we | ||
| /// then have to re-acquire. | ||
| /// | ||
| /// A file the client never opened is solved at `Require::Everything`, the |
There was a problem hiding this comment.
I think this docstring could be a lot less verbose
| // Bring in-memory content up to date. `didChange` skips this step when | ||
| // another mutation is already queued, so the overlay can be behind | ||
| // `open_files` even though the queue has drained. | ||
| transaction.set_memory( |
There was a problem hiding this comment.
this is the same as just running validate_in_memory_for_transaction, right? can we reuse that?
| Ok(true) | ||
| } | ||
| TSPRequests::ResolveImportRequest { params, .. } => { | ||
| self.handle_resolve_import( |
There was a problem hiding this comment.
I think we need to update this too if we want to be able to save the transactions from this call. It looks like it was missed because it already passed ide_transaction_manager.
73c0d64 to
2a401d2
Compare
This comment has been minimized.
This comment has been minimized.
2a401d2 to
ccdb63c
Compare
This comment has been minimized.
This comment has been minimized.
Summary:
TSP supports requests on files which have not yet been opened. In this case, we
would create a throw-away transaction that solved the module to
`Step::Solutions` with `Require::Everything`, ran the request handler, and
returned the result. Solving the module for every request is extremely
expensive, but all that work is thrown away between requests. Furthermore, TSP
is fairly chatty, sending hundreds (sometimes thousands) of `getComputedType`
requests.
This change offers a simple but effective solution: when handling a request for
a non-open file, we create a "possibly committable" transaction to raise the
require level for that handle. We are either able to (1) get a committable
transaction which updates the committed state, so future requests share the
earlier solve work, or (2) we get a non-committable transaction, but the
`TransactionManager` maintains a saved state, so that multiple requests can
still share solve work even when a background recheck is happening. This is also
how the Pyrefly LSP works.
To test this change I captured and replayed a Pylance session which had 43,811
`getComputedType` requests (89% of them through the extra connection). Request
costs compared to parent, in ms:
```
request target n | before p50 p90 total | after p50 p90 total
getComputedType non-open 22294 | 16.886 57.82 667.79s | 0.067 0.114 3.43s
getComputedType open 21517 | 0.046 0.09 1.25s | 0.059 0.074 1.34s
getSnapshot - 191 | 0.069 7.18 1.14s | 0.068 6.632 1.05s
resolveImport - 994 | 0.130 1.38 0.58s | 0.087 1.229 0.42s
total 670.95s total 6.41s
```
Both runs also carried an unrelated `demand` fix that is not part of this stack,
worth roughly 0.2s of the "after" total.
Test Plan:
Answers are unchanged. Response payloads carry allocation-order identifiers
that differ between runs, so responses are compared ignoring digits: 43822 of
43873 (99.88%) match. All 51 that differ are `-32802` snapshot-outdated
responses, never a differing type. Two runs of the same binary differ
structurally in 78 responses, so the gap between the two binaries is within a
single binary's run-to-run variation.
ccdb63c to
29906fe
Compare
|
According to mypy_primer, this change doesn't affect type check results on a corpus of open source code. ✅ |
Summary:
TSP supports requests on files which have not yet been opened. In this case, we
would create a throw-away transaction that solved the module to
Step::SolutionswithRequire::Everything, ran the request handler, andreturned the result. Solving the module for every request is extremely
expensive, but all that work is thrown away between requests. Furthermore, TSP
is fairly chatty, sending hundreds (sometimes thousands) of
getComputedTyperequests.
This change offers a simple but effective solution: when handling a request for
a non-open file, we create a "possibly committable" transaction to raise the
require level for that handle. We are either able to (1) get a committable
transaction which updates the committed state, so future requests share the
earlier solve work, or (2) we get a non-committable transaction, but the
TransactionManagermaintains a saved state, so that multiple requests canstill share solve work even when a background recheck is happening. This is also
how the Pyrefly LSP works.
To test this change I captured and replayed a Pylance session which had 43,811
getComputedTyperequests (89% of them through the extra connection). Requestcosts compared to parent, in ms:
Both runs also carried an unrelated
demandfix that is not part of this stack,worth roughly 0.2s of the "after" total.
Test Plan:
Answers are unchanged. Response payloads carry allocation-order identifiers
that differ between runs, so responses are compared ignoring digits: 43822 of
43873 (99.88%) match. All 51 that differ are
-32802snapshot-outdatedresponses, never a differing type. Two runs of the same binary differ
structurally in 78 responses, so the gap between the two binaries is within a
single binary's run-to-run variation.