Read metadata of installed packages lazily#1273
Conversation
| /// Mtime of the package's `DESCRIPTION` file, or [`FileRevision::zero`] | ||
| /// when it can't be stat'd. Drives the lazy [`Package::description`] |
There was a problem hiding this comment.
In the "cant be stat'd" case of returning zero, we could end up stuck with an outdated version of the file right? Like we read it once and never again because we never move off 0?
Is it better to force it to read it every time in the case where we cant determine a FileRevision?
(I have no idea if you can even do that with salsa's caching model)
There was a problem hiding this comment.
It should get refreshed by the scanner on the next file event, which is the ty model.
Claude says we could use this to force a reread of the query next time: https://github.com/salsa-rs/salsa/blob/86e69d7eacdbc4ee87e183a147af02ee5d932ca1/src/database.rs#L76-L80
There was a problem hiding this comment.
Ok let's try out the approach of reporting an untracked query if the stat failed. Untested and would be very unlikely in real usage though.
There was a problem hiding this comment.
Yea I'm not super worried about it, more just curious about how you can express this in salsa, so report-untracked-read is neat to learn about
| /// In-memory `NAMESPACE`, checked by [`Package::namespace`] before it | ||
| /// touches disk. Mirrors [`File::source_text_override`]: `None` means | ||
| /// "read from disk". The scanners always leave it `None`. It's the | ||
| /// injection point unit tests use to supply a namespace for a synthetic | ||
| /// path with no file behind it, and the natural hook for an editor | ||
| /// editing a package's `NAMESPACE` live. | ||
| #[returns(ref)] | ||
| pub version: Option<String>, | ||
| #[returns(ref)] | ||
| pub namespace: Namespace, | ||
| pub namespace_override: Option<Namespace>, |
There was a problem hiding this comment.
But not for description?
There was a problem hiding this comment.
Actually it seems nothing currently uses this
There was a problem hiding this comment.
Not needed for description rn
| use crate::Package; | ||
|
|
||
| #[salsa::tracked] | ||
| impl Package { |
There was a problem hiding this comment.
Maybe move all of pub struct Package { here?
There was a problem hiding this comment.
And merge with package_resolve.rs?
There was a problem hiding this comment.
I've moved the input to package.rs. I'd keep package_resolve.rs separate for symmetry with file_resolve.rs though.
| /// scanning a library with hundreds of packages, so we defer it. | ||
| /// | ||
| /// A missing or unparseable `NAMESPACE` yields an empty `Namespace`. | ||
| #[salsa::tracked(returns(ref))] |
There was a problem hiding this comment.
lru bound for this and description()?
There was a problem hiding this comment.
i have 600 packages installed fwiw 😬
There was a problem hiding this comment.
But those installed packages will not be queried if they are not part of the dependency graph of the workspace.
There was a problem hiding this comment.
When we port completions to salsa, we should make sure completing e.g. library() doesn't depend on package details besides the name. If we need more than the name, then we may want LRU.
| /// installed package's `NAMESPACE` eagerly would dominate the cost of | ||
| /// scanning a library with hundreds of packages, so we defer it. | ||
| /// | ||
| /// A missing or unparseable `NAMESPACE` yields an empty `Namespace`. |
There was a problem hiding this comment.
It's interesting to decide when to use -> Namespace vs -> Result<Namespace>
I generally agree with this approach for this one though
There was a problem hiding this comment.
Oh actually i see description() uses Option<Description>. Should we be consistent across these two methods?
There was a problem hiding this comment.
Since reading DESCRIPTION is important to get a version, and since versions are involved in what sources we get from oak_sources, I think I prefer not to use the empty file approach for that one.
| /// A narrow query over [`Package::description`]: editing `DESCRIPTION` | ||
| /// without changing `Version:` backdates here, so downstream isn't | ||
| /// disturbed. |
lionel-
force-pushed
the
oak/salsa-29-optional-contents
branch
from
cc27487 to
2e72224
Compare
lionel-
force-pushed
the
oak/salsa-30-lazy-package-metadata
branch
2 times, most recently
from
056ecde to
59eb794
Compare
lionel-
force-pushed
the
oak/salsa-29-optional-contents
branch
from
ab82cf4 to
ab16ff6
Compare
lionel-
force-pushed
the
oak/salsa-30-lazy-package-metadata
branch
from
bc0615d to
42ad025
Compare
2 participants
Branched from #1271
Progress towards #1212
Closes #1265
Uses same approach as file contents in #1271 to read
NAMESPACEandDESCRIPTIONlazily. This way the only thing that we need to initialise a library of installed packages are the folder names and the presence of a DESCRIPTION file. Unlike workspace packages, we don't read thePackagefield and just assume that a folder with a description file is indeed a package named after the folder name.This avoids I/O and parsing costs for installed packages that are not imported or mentioned in the workspace files.