Refactor file handling #269

BrknRobot · 2025-08-08T20:47:24Z

Goals

Centralize file read logic
Create a single owner of file content
Remove duplication in the path table
Preserve tracking of overridden files to enable future checks against missing items to determine which file overwrite is to blame

amtep · 2025-08-09T11:49:59Z

This PR gives a significant speedup (6.5s to 5.0s for ck3) and I can't figure out why.

amtep · 2025-08-09T11:59:19Z

There's something wrong with the loca parsing, which is probably why it's faster. There are no reports about localization files in the output.

amtep · 2025-08-09T12:46:34Z

I got as far as discovering that the dummy localization files from mod-test-vanilla (such as dummy_l_english.yml) are never passed to the localization FileHandler. This leaves mod_langs empty.

BrknRobot · 2025-08-09T13:44:46Z

Oop, I'll track that down. Any design concerns?

amtep · 2025-08-09T19:46:00Z

I was concerned about possible slowdown due to all the locking around Arc<FileEntry>, which is why I tried to benchmark it :)

In one of my early designs before the PathTable, I tried to make pathname in Loc an Arc, and it resulted in significant slowdown.

Other than that, I'll have to read it a few more times.

src/fileset.rs

BrknRobot · 2025-08-10T02:18:20Z

I've made sure that the Arc is only cloned when needed. Most places pass the file entry around as &Arc<FileEntry> which allows the callee to decide if they need to increase the ref count, and only one place outside of building the fileset actually does this. Dereferencing to just read a field doesn't require any atomic operations.

My most recent tests show very similar performance to before this change. Some area's have improved (notably Errors.cache.get_line()) while others have regressed slightly. Though as I continue to cleanup the change set, there seem to be fewer regressions. I keep finding extra PathBuf clones that don't need to happen.

Clean up Fileset construction and fix bug Don't bother normalising paths fix tests

amtep

Mostly comment changes, but the use of PathIdx::UNTRACKED needs to be revisited. The entries that use it can definitely have errors reported. The launcher file via errors emitted by the parser, and the internal strings sometimes have errors that I need to fix. It would be awkward to not get those errors.

amtep · 2025-08-10T14:27:20Z

src/everything.rs

    /// and examine the mod files.
    ///
    /// `mod_root` is the path to the mod files. The config file will also be looked for there.
    ///


Function comment needs updating now

amtep · 2025-08-10T14:31:00Z

src/files/filecontent.rs

+#[derive(Debug)]
+pub struct FileContent {
+    /// Full content of the file. It must never be moved or modified.
+    // full: Cow<'static, str>,


stray commented-out line

amtep · 2025-08-10T14:31:49Z

src/files/filecontent.rs

+impl FileContent {
+    pub fn new(mut s: String) -> Self {
+        s.shrink_to_fit();
+        //Self { full: Cow::from(s), lines: OnceLock::new() }


Stray commented-out line

amtep · 2025-08-10T14:32:27Z

src/files/filecontent.rs

+    }
+
+    pub fn new_static(s: &'static str) -> Self {
+        //Self { full: Cow::from(s), lines: OnceLock::new() }


Stray commented-out line

amtep · 2025-08-10T14:40:46Z

src/files/filedb.rs

+
+impl FileDb {
+    /// Gets a reference to a file entry.
+    /// If the fileset has not been finalized, a new entry will be created and returned if needed.


"If the fileset has not been finalized" is no longer accurate

Also, "gets a reference" was a bit confusing because I read it as "receives" but you meant "fetches"

amtep · 2025-08-10T15:12:44Z

src/files/fileset.rs

        }
    }

+    /// # Panics


Same as above

amtep · 2025-08-10T15:13:30Z

src/files/fileset.rs

                        let msg = format!("file in unexpected directory {}", dirname.display());
                        let info = format!("did you mean common/{} ?", dirname.display());
-                        err(ErrorKey::Filename).msg(msg).info(info).loc(entry).push();
+                        err(ErrorKey::Filename).msg(msg).info(info).loc(&**entry).push();


Is the &** still needed? You added a From for the Arc

amtep · 2025-08-10T15:13:47Z

src/files/fileset.rs

    }

+    /// # Panics
+    /// Will panic if a lock on self.used can not be obtained


Comment is inaccurate :) See above

amtep · 2025-08-10T15:24:24Z

src/files/fileset_builder.rs

+            self.db.files.entry(entry.path().to_path_buf()).or_insert_with(|| {
+                FileEntry::new(inner_path.to_path_buf(), kind, entry.path().to_path_buf())


Calling entry.path().to_path_buf() twice here

amtep · 2025-08-10T15:37:35Z

src/parse/pdxfile.rs

-    let content = leak(content);
-    store_source_file(entry.fullpath().to_path_buf(), &content[offset..]);
-    parse_pdx(entry, &content[offset..], parser)
+pub fn parse_pdx_file(entry: &FileEntry, parser: &ParserMemory) -> Option<Block> {


I guess this function can be removed and the callers just call parse_pdx

amtep · 2025-08-12T09:42:02Z

Btw I've been thinking that the internal pdx script may have been a mistake and a table-based approach would be better. But getting rid of them is a longer term task.

(Reasoning: errors in the pdx script cause runtime failures, which I mjght not catch before release. This has happened once.)

fix: criterion benchmark paths

e321685

amtep reviewed Aug 9, 2025

View reviewed changes

src/fileset.rs Outdated Show resolved Hide resolved

BrknRobot force-pushed the refactor_fileset branch from 8d879e4 to 0465025 Compare August 10, 2025 02:45

refactor: Give ownership of file content to FileEntry

d66848f

Clean up Fileset construction and fix bug Don't bother normalising paths fix tests

BrknRobot force-pushed the refactor_fileset branch from 0465025 to d66848f Compare August 10, 2025 03:22

amtep requested changes Aug 10, 2025

View reviewed changes

		self.db.files.entry(entry.path().to_path_buf()).or_insert_with(\|\| {
		FileEntry::new(inner_path.to_path_buf(), kind, entry.path().to_path_buf())

Refactor file handling #269

Are you sure you want to change the base?

Refactor file handling #269

Uh oh!

Conversation

BrknRobot commented Aug 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtep commented Aug 9, 2025

Uh oh!

amtep commented Aug 9, 2025

Uh oh!

amtep commented Aug 9, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

BrknRobot commented Aug 9, 2025

Uh oh!

amtep commented Aug 9, 2025

Uh oh!

Uh oh!

BrknRobot commented Aug 10, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

amtep left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

amtep commented Aug 12, 2025

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

BrknRobot commented Aug 8, 2025 •

edited

Loading

amtep commented Aug 9, 2025 •

edited

Loading

BrknRobot commented Aug 10, 2025 •

edited

Loading