Replace singleton file overrides with granular inputs and views

[integraledelebesgue]

Summary

A new architecture of file inputs, which significantly improves the memory performance of CairoLS.

Changes

• File overrides don't exist anymore. Instead, all file contents (physical and generated) are stored in a shared map
• The shared map is registered as a Salsa View, which allows for granular, incremental computation for single files

---

Type of change

Please check one:

• Bug fix (fixes incorrect behavior)
• New feature
• Performance improvement
• Documentation change with concrete technical impact
• Style, wording, formatting, or typo-only change

[reviewable-StarkWare]

This change is 

[integraledelebesgue]

This change is part of the following stack:

• Optimize diagnostics flattening in get_all #9828
  • Skip modules without priv uses when computing unused imports diagnostics #9829
    • Replace singleton file overrides with granular inputs and views #9830 ◀
      • Replace singleton crate configs input with granular inputs and views #9831
        • Replace singleton plugin overrides with granular inputs and views #9835

_{Change managed by git-spice.}

[piotmag769]

@piotmag769 reviewed 13 files and all commit messages, and made 7 comments.
Reviewable status: all files reviewed, 7 unresolved discussions (waiting on integraledelebesgue and orizi).

---

crates/cairo-lang-filesystem/src/db.rs line 237 at r2 (raw file):

    #[returns(ref)]
    pub crate_configs: Option<OrderedHashMap<CrateInput, CrateConfigurationInput>>,
    /// Structural revision for databases that source file content from granular per-file slots

Doesn't explain why we need this

---

crates/cairo-lang-filesystem/src/db.rs line 268 at r2 (raw file):

pub type FileContentStorage = Arc<RwLock<HashMap<FileInput, FileContents>>>;

pub fn new_file_content_storage() -> FileContentStorage {

This is useless

---

crates/cairo-lang-filesystem/src/db.rs line 259 at r2 (raw file):

/// invalidation.
#[salsa::input]
pub struct FileContents {

Do we need two fields? Only one is set at a time anyways, the other has to be None

---

crates/cairo-lang-filesystem/src/db.rs line 499 at r2 (raw file):

}

fn file_contents_storage(db: &dyn Database) -> &FileContentStorage {

Inline

---

crates/cairo-lang-filesystem/src/db.rs line 555 at r2 (raw file):

pub type FileContentSnapshot = OrderedHashMap<FileInput, (Option<Arc<str>>, Option<Arc<str>>)>;

pub fn snapshot_file_contents(db: &dyn Database) -> FileContentSnapshot {

Where is it needed? Also doc

---

crates/cairo-lang-filesystem/src/db.rs line 300 at r2 (raw file):

}

fn cast_file_content_view<Db: FileContentView + 'static>(

Put this next to register_files_group_view or preferably declare inside it

---

crates/cairo-lang-filesystem/src/db.rs line 657 at r2 (raw file):

#[salsa::tracked(returns(ref))]
fn file_content<'db>(db: &'db dyn Database, file_id: FileId<'db>) -> Option<Arc<str>> {
    let _ = files_group_input(db).file_contents_revision(db);

Please doc why

[orizi]

@orizi made 1 comment.
Reviewable status: all files reviewed, 8 unresolved discussions (waiting on integraledelebesgue).

---

a discussion (no related file):
without a full documentation of what you are trying to do it and how and why it helps - would definitely not approve the PR.

[Arcticae]

@Arcticae reviewed 9 files and all commit messages, and made 6 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on integraledelebesgue and piotmag769).

---

crates/cairo-lang-filesystem/src/db.rs line 268 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This is useless

Also Default should be derivable here

---

crates/cairo-lang-filesystem/src/db.rs line 657 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Please doc why

Mechanism of invalidation should be described here (on fist file edit vs later edits)

---

crates/cairo-lang-filesystem/src/db.rs line 274 at r2 (raw file):

/// Database view used by [`FilesGroup::file_content`] to retrieve per-file editor/generated
/// content.
pub trait FileContentView: Database {

Describe why a view is used

---

crates/cairo-lang-filesystem/src/db.rs line 517 at r2 (raw file):

}

pub fn set_on_disk_file_content(

This one is unused i think

---

crates/cairo-lang-filesystem/src/db.rs line 526 at r2 (raw file):

}

pub fn set_on_disk_file_content_for_input(

override terminology was pretty good one and i think it still describes what we want to achieve here

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

) {
    let handle = ensure_file_contents_handle_for_input(db, file_input);
    handle.set_generated_content(db).with_durability(Durability::HIGH).to(content.map(ArcStr::new));

Why is durability value different here?

[piotmag769]

@piotmag769 made 3 comments.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on Arcticae and integraledelebesgue).

---

crates/cairo-lang-filesystem/src/db.rs line 268 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Also Default should be derivable here

Wdym? The default is there since FileContentStorage is a only type alias

---

crates/cairo-lang-filesystem/src/db.rs line 274 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Describe why a view is used

I am not sure wdym by that - it is the same as other traits that have Database as supertrait. There is nothing salsa view specific in this trait

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Why is durability value different here?

Generated content is set rarely - e.g. we do that when there are integration test in Scarb package without tests/lib.cairo file - in that case we set generated content for tests/lib.cairo. I still believe there shouldn't se a separate field for that in FileContents, but a helper with different durability is definitely useful

[wawel37]

@wawel37 made 10 comments.
Reviewable status: 3 of 13 files reviewed, 12 unresolved discussions (waiting on Arcticae, integraledelebesgue, and piotmag769).

---

crates/cairo-lang-filesystem/src/db.rs line 237 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Doesn't explain why we need this

Done

---

crates/cairo-lang-filesystem/src/db.rs line 259 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Do we need two fields? Only one is set at a time anyways, the other has to be None

Done

---

crates/cairo-lang-filesystem/src/db.rs line 268 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Wdym? The default is there since FileContentStorage is a only type alias

Done

---

crates/cairo-lang-filesystem/src/db.rs line 300 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Put this next to register_files_group_view or preferably declare inside it

Done

---

crates/cairo-lang-filesystem/src/db.rs line 499 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Inline

Done

---

crates/cairo-lang-filesystem/src/db.rs line 517 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

This one is unused i think

Done

---

crates/cairo-lang-filesystem/src/db.rs line 526 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

override terminology was pretty good one and i think it still describes what we want to achieve here

Done

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Generated content is set rarely - e.g. we do that when there are integration test in Scarb package without tests/lib.cairo file - in that case we set generated content for tests/lib.cairo. I still believe there shouldn't se a separate field for that in FileContents, but a helper with different durability is definitely useful

Added proper comment for that

---

crates/cairo-lang-filesystem/src/db.rs line 555 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Where is it needed? Also doc

It's needed for linter fixer

---

crates/cairo-lang-filesystem/src/db.rs line 657 at r2 (raw file):

Previously, Arcticae (Tomasz Rejowski) wrote…

Mechanism of invalidation should be described here (on fist file edit vs later edits)

Done

[piotmag769]

@piotmag769 reviewed 10 files and all commit messages, made 7 comments, and resolved 6 discussions.
Reviewable status: all files reviewed, 12 unresolved discussions (waiting on Arcticae and integraledelebesgue).

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

Previously, wawel37 (Mateusz Kowalski) wrote…

Added proper comment for that

The comment does not reflect what I wrote ;p Your current comment is wrong, this is not used for plugin generated content. Please change

---

crates/cairo-lang-filesystem/src/db.rs line 240 at r4 (raw file):

    /// file. The [`file_content`] tracked function reads this field so that it re-executes when a
    /// previously-unregistered file gets a handle — at which point it records the per-file
    /// dependency for future fine-grained incremental updates.

Imprecise.Dependency on particular FileContents is registered by file_content when the tracked function reads the field of this FileContents

Suggestion:

    /// previously-unregistered file gets a handle.

---

crates/cairo-lang-filesystem/src/db.rs line 260 at r4 (raw file):

/// Per-file content slot used by long-running tools (like CairoLS) that need granular
/// invalidation. A file is either an on-disk override or a generated file — never both — so a
/// single field is sufficient.

Sth like that is more precise

Suggestion:

/// Input used for overriding a content of a single file.
/// Useful for tools like CairoLS to manage contents of files that are opened in the editor.
///
/// Notice that this structure holds content override for a single file only - not the whole hashmap of overrides for all overriden files.
/// This is done to ensure granular invalidation of results of `file_content` tracked function - so that we don't invalidate everything when a single override changes.

---

crates/cairo-lang-filesystem/src/db.rs line 262 at r4 (raw file):

/// single field is sufficient.
#[salsa::input]
pub struct FileContents {

More precise name imo - I think all the names should be changes accordingly

Suggestion:

FileContentOverride

---

crates/cairo-lang-filesystem/src/db.rs line 500 at r4 (raw file):

}

/// Overrides the on-disk content of a file as seen by the compiler. Used by editors to show

Suggestion:

Used by CairoLS to make compiler aware of

---

crates/cairo-lang-filesystem/src/db.rs line 508 at r4 (raw file):

) {
    let handle = ensure_file_contents_handle_for_input(db, file_input);
    // LOW durability: on-disk overrides change on any even minor file changes.

Suggestion:

// LOW durability: on-disk overrides change frequently for files opened in editor in CairoLS.
// For regular compiling use cases the durability does not matter, as the inputs do not change during compilation.

---

crates/cairo-lang-filesystem/src/db.rs line 631 at r4 (raw file):

    let _ = files_group_input(db).file_contents_revision(db);
    maybe_file_content_view(db)
        .and_then(|view| view.file_content_override(file_id))

WDYT? This saves invalidation of this tracked function for FileId for which there already exists FileContents in the FileContentStorage.

Explanation: we need the dependency on file_contents_revision only for FileId which is not in the FileContentStorage map. Such a file has to depend on file_content_revision that we bump on each new FileContents creation - the newly created input (content override) may have been for the aforementioned file. For files which already have their FileContents - we do not care about creation of new FileContents - they already depend on their own FileContents.

Suggestion:

    maybe_file_content_view(db)
        .and_then(|db| {
            let file_input = db.file_input(file_id);
            if let Some(file_contents) = db.file_contents_for_input(file_input) else {
                file_contents.content(db)
            } else {
                let _ = files_group_input(db).file_contents_revision(db);
                None
            }
        })

[wawel37]

@wawel37 made 7 comments.
Reviewable status: 12 of 13 files reviewed, 12 unresolved discussions (waiting on Arcticae, integraledelebesgue, orizi, and piotmag769).

---

a discussion (no related file):

Previously, orizi wrote…

without a full documentation of what you are trying to do it and how and why it helps - would definitely not approve the PR.

Added extended docs to the Code. Lmk if I should extend the docs more

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

The comment does not reflect what I wrote ;p Your current comment is wrong, this is not used for plugin generated content. Please change

Done, sorry, replied to wrong comment. Anyway, changed the comments here also.

---

crates/cairo-lang-filesystem/src/db.rs line 240 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Imprecise.Dependency on particular FileContents is registered by file_content when the tracked function reads the field of this FileContents

Done

---

crates/cairo-lang-filesystem/src/db.rs line 260 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

Sth like that is more precise

Done

---

crates/cairo-lang-filesystem/src/db.rs line 262 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

More precise name imo - I think all the names should be changes accordingly

Done

---

crates/cairo-lang-filesystem/src/db.rs line 500 at r4 (raw file):

}

/// Overrides the on-disk content of a file as seen by the compiler. Used by editors to show

Done

---

crates/cairo-lang-filesystem/src/db.rs line 508 at r4 (raw file):

) {
    let handle = ensure_file_contents_handle_for_input(db, file_input);
    // LOW durability: on-disk overrides change on any even minor file changes.

Done

[piotmag769]

@piotmag769 reviewed 1 file and all commit messages, made 1 comment, and resolved 6 discussions.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on Arcticae, integraledelebesgue, and orizi).

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

Previously, wawel37 (Mateusz Kowalski) wrote…

Done, sorry, replied to wrong comment. Anyway, changed the comments here also.

This is still wrong - in LS it can change more frequently than this. I would just say that it is set rarely - to make compiler act like there exists a cairo file (usually lib.cairo) while this file does not actually exists on the disk.

[integraledelebesgue]

@integraledelebesgue made 2 comments.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on Arcticae, orizi, and piotmag769).

---

crates/cairo-lang-filesystem/src/db.rs line 274 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

I am not sure wdym by that - it is the same as other traits that have Database as supertrait. There is nothing salsa view specific in this trait

Right, nothing related to Salsa view. This trait extracts the common logic of access to the storage, which is db-specific (each db has its own field). We simply have to implement the getter and get the rest of the logic implemented

---

crates/cairo-lang-filesystem/src/db.rs line 631 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

WDYT? This saves invalidation of this tracked function for FileId for which there already exists FileContents in the FileContentStorage.

Explanation: we need the dependency on file_contents_revision only for FileId which is not in the FileContentStorage map. Such a file has to depend on file_content_revision that we bump on each new FileContents creation - the newly created input (content override) may have been for the aforementioned file. For files which already have their FileContents - we do not care about creation of new FileContents - they already depend on their own FileContents.

I'm not sure if Salsa handles the dependency on file_contents_revision per-entry or per-query. The conditional dependency only makes sense if the former is true

[piotmag769]

@piotmag769 made 1 comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on Arcticae, integraledelebesgue, and orizi).

---

crates/cairo-lang-filesystem/src/db.rs line 631 at r4 (raw file):
It is per entry - it would be weird for it to be otherwise

https://salsa-rs.github.io/salsa/reference/algorithm.html#basic-rule-when-inputs-change-re-execute

When you invoke a tracked function, in addition to storing the value that was returned, we also track what other tracked functions it depends on, and the revisions when their value last changed. When you invoke the function again, if the database is in a new revision, then we check whether any of the inputs to this function have changed in that new revision. If not, we can just return our cached value.

[integraledelebesgue]

@integraledelebesgue made 1 comment.
Reviewable status: all files reviewed, 6 unresolved discussions (waiting on Arcticae, orizi, and piotmag769).

---

crates/cairo-lang-filesystem/src/db.rs line 631 at r4 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

It is per entry - it would be weird for it to be otherwise

https://salsa-rs.github.io/salsa/reference/algorithm.html#basic-rule-when-inputs-change-re-execute

When you invoke a tracked function, in addition to storing the value that was returned, we also track what other tracked functions it depends on, and the revisions when their value last changed. When you invoke the function again, if the database is in a new revision, then we check whether any of the inputs to this function have changed in that new revision. If not, we can just return our cached value.

So I think it's correct to do so

[wawel37]

@wawel37 made 2 comments.
Reviewable status: 12 of 13 files reviewed, 6 unresolved discussions (waiting on Arcticae, orizi, and piotmag769).

---

crates/cairo-lang-filesystem/src/db.rs line 550 at r2 (raw file):

Previously, piotmag769 (Piotr Magiera) wrote…

This is still wrong - in LS it can change more frequently than this. I would just say that it is set rarely - to make compiler act like there exists a cairo file (usually lib.cairo) while this file does not actually exists on the disk.

Done

---

crates/cairo-lang-filesystem/src/db.rs line 631 at r4 (raw file):

Previously, integraledelebesgue (Jan Smółka) wrote…

So I think it's correct to do so

Done

[piotmag769]

@piotmag769 reviewed 1 file and all commit messages, made 1 comment, and resolved 1 discussion.
Reviewable status: all files reviewed, 5 unresolved discussions (waiting on Arcticae and orizi).

[orizi]

@orizi reviewed all commit messages, made 7 comments, and resolved 1 discussion.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on Arcticae and integraledelebesgue).

---

a discussion (no related file):
@eytan-starkware

---

crates/cairo-lang-filesystem/src/db.rs line 239 at r6 (raw file):

    /// Structural revision bumped each time a new [`FileContentOverride`] handle is first created
    /// for a previously-unregistered file.
    pub file_contents_revision: u64,

why isn't salsa any internal revision any better?

add this somewhere at the doc.

---

crates/cairo-lang-filesystem/src/db.rs line 279 at r6 (raw file):

/// Implement this on the concrete DB struct and register it with [`register_files_group_view`].
pub trait FileContentView: Database {
    fn file_content_storage(&self) -> Option<&FileContentStorage> {

doc fn

---

crates/cairo-lang-filesystem/src/db.rs line 283 at r6 (raw file):

    }

    fn file_contents_for_input(&self, file_input: &FileInput) -> Option<FileContentOverride> {

doc fn

---

crates/cairo-lang-filesystem/src/db.rs line 294 at r6 (raw file):

}

fn file_content_view(db: &dyn Database) -> &dyn FileContentView {

doc fn

---

crates/cairo-lang-filesystem/src/db.rs line 303 at r6 (raw file):

}

fn maybe_file_content_view(db: &dyn Database) -> Option<&dyn FileContentView> {

doc fn

---

crates/cairo-lang-filesystem/src/db.rs line 391 at r6 (raw file):

}

fn bump_file_contents_revision(db: &mut dyn Database) {

doc fn

[wawel37]

@wawel37 made 6 comments.
Reviewable status: 12 of 13 files reviewed, 10 unresolved discussions (waiting on Arcticae, orizi, and piotmag769).

---

crates/cairo-lang-filesystem/src/db.rs line 239 at r6 (raw file):

Previously, orizi wrote…

why isn't salsa any internal revision any better?

add this somewhere at the doc.

Done, added docs explaining why we do it this way

---

crates/cairo-lang-filesystem/src/db.rs line 279 at r6 (raw file):

Previously, orizi wrote…

doc fn

Done

---

crates/cairo-lang-filesystem/src/db.rs line 283 at r6 (raw file):

Previously, orizi wrote…

doc fn

Done

---

crates/cairo-lang-filesystem/src/db.rs line 294 at r6 (raw file):

Previously, orizi wrote…

doc fn

Done

---

crates/cairo-lang-filesystem/src/db.rs line 303 at r6 (raw file):

Previously, orizi wrote…

doc fn

Done

---

crates/cairo-lang-filesystem/src/db.rs line 391 at r6 (raw file):

Previously, orizi wrote…

doc fn

Done

[piotmag769]

@piotmag769 reviewed 1 file and all commit messages.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on Arcticae and orizi).