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
            }
        })

[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).

[eytan-starkware]

Have you considered moving all file contents out of the DB, and having the DB "start" from the AST?
I am essentially suggesting "handle all parsing before the DB, and have the DB only track AST nodes" which I suppose might improve stability, but will mostly improve code.

@eytan-starkware made 1 comment.
Reviewable status: all files reviewed, 10 unresolved discussions (waiting on Arcticae and orizi).

[wawel37]

@eytan-starkware

1. Storing the file content inside the DB is very convenient for cairols, as we change the file content really frequently. Having the parsing mechanism outside of the DB would make the API of the outside parser much more complicated and harder to maintain. So the code would not be cleaner this way.
2. The parsing of virtual files would be much more complicated, as we need the built macro code from virtual files to create a correct AST. So with parsing outside the DB, we would have to first parse the code, pass the AST to DB and later let it expand the macros. Then again we would go back to parsing again, as the code changed because of the virtual files. I got a feeling that might not be a good solution.
3. Also we would have to probably move some other things that the parsing depends on out of the DB, like: crate's edition and feature flags (as far as i know, they are stored inside the db).

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

[eytan-starkware]

@wawel37
3. Details are wrong, but I will accept that it is out of scope at the moment. It is a much cleaner approach to pull parsing out of DB IMO and you should consider it. Changing a file without changing the ast form (comments, renames, and such) should be easier to deal with this way.

I am really not convinced about 1. You would always parse again, I see no real gain in having it inside the DB (as it is fast, and happens often).
2. Parsing virtual files would just mean that processing a macro as a query would include parsing. I am not seeing a problem with that.

@eytan-starkware reviewed 3 files and all commit messages, and made 4 comments.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on Arcticae, integraledelebesgue, and orizi).

---

crates/cairo-lang-compiler/src/db.rs line 116 at r7 (raw file):

    /// Snapshots the db for read only.
    pub fn snapshot(&self) -> RootDatabase {
        RootDatabase { storage: self.storage.clone(), file_contents: self.file_contents.clone() }

Snapshot should be readonly, but now we are exposing a writable file_contents. What will happen if you mutate inputs on the snapshot/origianl while processing the other?

---

crates/cairo-lang-compiler/src/project.rs line 68 at r7 (raw file):

        }
        let file_input = {
            let crate_id = CrateId::plain(db, SmolStrId::from(db, file_stem));

Create crate_id once

---

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

    /// Returns the [`FileContentStorage`] side-table, or `None` if this database does not hold
    /// file content overrides.
    fn file_content_storage(&self) -> Option<&FileContentStorage> {

Why is this function returning an Option? All impls return Some

[wawel37]

@eytan-starkware You might be right all along. I'm just curious if we could accept those changes right now as they are, and we will implement yours approach and tests all the performances later on, as we've already measured the gains from this changes and they are really beneficial for us. What do you think?

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

[eytan-starkware]

That is essentially what I said, so yes.

@eytan-starkware made 1 comment.
Reviewable status: all files reviewed, 13 unresolved discussions (waiting on Arcticae, integraledelebesgue, and orizi).

[wawel37]

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

---

crates/cairo-lang-compiler/src/db.rs line 116 at r7 (raw file):

Previously, eytan-starkware wrote…

Snapshot should be readonly, but now we are exposing a writable file_contents. What will happen if you mutate inputs on the snapshot/origianl while processing the other?

Done

---

crates/cairo-lang-compiler/src/project.rs line 68 at r7 (raw file):

Previously, eytan-starkware wrote…

Create crate_id once

Done

---

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

Previously, eytan-starkware wrote…

Why is this function returning an Option? All impls return Some

Done

[eytan-starkware]

@eytan-starkware reviewed 3 files, made 4 comments, and resolved 1 discussion.
Reviewable status: 5 of 13 files reviewed, 15 unresolved discussions (waiting on Arcticae, integraledelebesgue, orizi, and piotmag769).

---

crates/cairo-lang-compiler/src/db.rs line 116 at r7 (raw file):

Previously, wawel37 (Mateusz Kowalski) wrote…

Done

Please explain why we need a lock

---

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

#[salsa::input]
// TODO(eytan-starkware): Change this mechanism to hold input handles on the db struct outside

You are essentially doing this todo, but differently then intended, so I want to understand the cause. Why do we need file_contents_revision instead of having the DB struct (e.g. RootDatabase) have input handles on it, and use the set functions to update the input? As I understand your main concern is a missing file, but that is solvable with things like db.runtime().report_untracked_read()? Or even better when you need an input you pass an input handle.

It seems to me like you are reimplementing parts of salsa tracking, but it is unclear to me why. What is the limitation?

---

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

pub struct FileContentOverride {
    #[returns(ref)]
    pub content: Option<ArcStr>,

Do we actually care if it is an override or not? I think this is transparent for the compiler, and a file can jsut be text. If you want option to support missing files that could be fine.

---

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

/// created with `&mut dyn Database` while existing ones are read from within tracked functions
/// via [`FileContentView`].
pub type FileContentStorage = Arc<RwLock<HashMap<FileInput, FileContentOverride>>>;

Maybe you can remove the lock now that snapshot is deepcopy?

[eytan-starkware]

@eytan-starkware made 2 comments and resolved 1 discussion.
Reviewable status: 5 of 13 files reviewed, 16 unresolved discussions (waiting on Arcticae, integraledelebesgue, orizi, and piotmag769).

---

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

    /// Returns the overridden content for the given file, if one exists.
    fn file_content_override<'db>(&'db self, file_id: FileId<'db>) -> Option<&'db ArcStr> {

Is file_content_override ever used?

---

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

        .and_then(|view| {
            let file_input = view.file_input(file_id);
            if let Some(file_contents) = view.file_contents_for_input(file_input) {

You already have a helper ffunction for reading content from file_input

[eytan-starkware]

@eytan-starkware made 4 comments.
Reviewable status: 5 of 13 files reviewed, 20 unresolved discussions (waiting on Arcticae, integraledelebesgue, orizi, and piotmag769).

---

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

fn file_content_view(db: &dyn Database) -> &dyn FileContentView {
    let caster =
        catch_unwind(AssertUnwindSafe(|| *views(db).downcaster_for::<dyn FileContentView>())).ok();

You could just have this function return error/option instead of crashing? That way we could return nicer diagnostics (even if it is an internal error)

---

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

/// [`register_files_group_view`] was not called for this database type.
fn maybe_file_content_view(db: &dyn Database) -> Option<&dyn FileContentView> {
    catch_unwind(AssertUnwindSafe(|| file_content_view(db))).ok()

Wouldnt this be the function returning Option and the previous one using this one?

---

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

}

fn ensure_file_contents_handle_for_input(

doc and maybe rename

---

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

    let handle = ensure_file_contents_handle_for_input(db, file_input);
    // HIGH durability: set rarely.
    handle.set_content(db).with_durability(Durability::HIGH).to(content.map(ArcStr::new));

Setting high durability wont miss when generated content changes due to changes in files with low durability? I am pretty sure it will

[wawel37]

@wawel37 made 10 comments.
Reviewable status: 3 of 13 files reviewed, 20 unresolved discussions (waiting on Arcticae, eytan-starkware, orizi, and piotmag769).

---

crates/cairo-lang-compiler/src/db.rs line 116 at r7 (raw file):

Previously, eytan-starkware wrote…

Please explain why we need a lock

Done. Added additional comments explaining that.

---

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

Previously, eytan-starkware wrote…

You are essentially doing this todo, but differently then intended, so I want to understand the cause. Why do we need file_contents_revision instead of having the DB struct (e.g. RootDatabase) have input handles on it, and use the set functions to update the input? As I understand your main concern is a missing file, but that is solvable with things like db.runtime().report_untracked_read()? Or even better when you need an input you pass an input handle.

It seems to me like you are reimplementing parts of salsa tracking, but it is unclear to me why. What is the limitation?

Done, used the report_untracked_read instead.

---

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

Previously, eytan-starkware wrote…

Do we actually care if it is an override or not? I think this is transparent for the compiler, and a file can jsut be text. If you want option to support missing files that could be fine.

Changed the name but the content stays optional due to some potential cairoLS application for that.

---

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

Previously, eytan-starkware wrote…

Maybe you can remove the lock now that snapshot is deepcopy?

The lock is for clone(), not snapshot() — parallel workers share the same Arc and may read while the main thread writes.

---

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

Previously, eytan-starkware wrote…

Is file_content_override ever used?

Renamed to content_for_input and it's now used inside file_content.

---

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

Previously, eytan-starkware wrote…

You could just have this function return error/option instead of crashing? That way we could return nicer diagnostics (even if it is an internal error)

I've added some context for the crash. I thought about adding some diagnostics but in my opinion showing it to the user is useless, as if it really crashes, it's 100% dev fault, not the user (forgot to call register_files_group_view during db init).

---

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

Previously, eytan-starkware wrote…

Wouldnt this be the function returning Option and the previous one using this one?

Done — maybe_file_content_view now does the actual downcast and returns Option, file_content_view just wraps it with expect.

---

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

Previously, eytan-starkware wrote…

doc and maybe rename

Done

---

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

Previously, eytan-starkware wrote…

Setting high durability wont miss when generated content changes due to changes in files with low durability? I am pretty sure it will

No matter the durability, it's impossible for salsa to miss something due to anything being high or low durability.

From the Salsa source (src/durability.rs): the skip only fires when "the only changes were to inputs of low durability" — so when
set_generated_file_content_for_input mutates a HIGH durability input, Salsa always re-validates dependents. No correctness issue.

---

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

Previously, eytan-starkware wrote…

You already have a helper ffunction for reading content from file_input

Done.

[piotmag769]

@piotmag769 reviewed 11 files and all commit messages.
Reviewable status: 12 of 13 files reviewed, 20 unresolved discussions (waiting on Arcticae, eytan-starkware, and orizi).