Make File contents lazy#1271
Conversation
|
I have not fully read this yet but my brain is also thinking about #1266 (comment) and the fact that we are going to be doing a full workspace index (which means reading contents from disk for every file) on startup. So....is it even lazy anymore? Is it mostly for non workspace |
|
Ok I also see it is not just about being lazy from a performance POV It is probably mostly about gaining that |
|
Should |
| pub(crate) fn contents<'db>(&self, db: &'db dyn ArkDb) -> &'db str { | ||
| self.file.contents(db).as_str() | ||
| self.file.source_text(db).as_str() | ||
| } |
There was a problem hiding this comment.
Also make this source_text() for consistency?
There was a problem hiding this comment.
Good idea, I'll do it in the OpenFile PR
| // Clear the editor override so the disk contents becomes the source of | ||
| // truth again. This immediately invalidates queries that depend on | ||
| // `source_text`, because the latter depends on the override. | ||
| file.set_source_text_override(self).to(None); |
There was a problem hiding this comment.
👍 i was explicitly looking for this, nice
lionel-
force-pushed
the
oak/salsa-28-rtask-unwind
branch
from
a05a82d to
1705d42
Compare
lionel-
force-pushed
the
oak/salsa-29-optional-contents
branch
from
cc27487 to
2e72224
Compare
I don't think so, this needs to stay warm to timely answer workspace queries from the user. In general I think of LRU mostly as a safeguard for data about installed packages, once they're hooked up to oak_sources. We can revisit if we see issues in large workspaces though. |
2 participants
Branched from #1269
Progress towards #1212
This PR implements ty's lazy model for storing sources. This should help keep memory under control, especially when we integrate library packages with the
oak_sourcescache.Fileno longer stores the source string as a Salsa input, instead it includes arevisionfield, an optional override for when you do want to store a source (e.g. virtual files and open editors), and a lazysource_text()tracked query that reads from disk if needed. Failures return empty sources with an error logged, so the model degrades to empty contents. The query stores at most 128 sources in the Salsa cache, following our storage retention for parse trees.oak_scanreads the file'smtimeinstead of the contents and store that as revision. The watcher's Changed/Created events bump the revision.This makes the Salsa model a bit impure. If we miss a FS event, or it's lagging a bit, and re-read from disk at that point, we'll get invalid contents. It becomes possible for different queries to be inconsistent. Should be self-healing as long as we do get the events eventually. Since ty does this, I'm not too worried about this approach. The alternative is that we'll have to store all the sources of the entire dependency tree, which is likely a bit much especially if there are many Ark sessions live in the workspace (e.g. multiple notebooks, shiny apps, etc).