Conversation
|
This PR gives a significant speedup (6.5s to 5.0s for ck3) and I can't figure out why. |
|
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. |
|
I got as far as discovering that the dummy localization files from |
|
Oop, I'll track that down. Any design concerns? |
|
I was concerned about possible slowdown due to all the locking around 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. |
|
I've made sure that the Arc is only cloned when needed. Most places pass the file entry around as My most recent tests show very similar performance to before this change. Some area's have improved (notably |
BrknRobot
force-pushed
the
refactor_fileset
branch
from
8d879e4 to
0465025
Compare
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
amtep
left a comment
amtep
left a comment
There was a problem hiding this comment.
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.
| @@ -234,15 +231,11 @@ impl Everything { | |||
| /// | |||
There was a problem hiding this comment.
Function comment needs updating now
| #[derive(Debug)] | ||
| pub struct FileContent { | ||
| /// Full content of the file. It must never be moved or modified. | ||
| // full: Cow<'static, str>, |
| impl FileContent { | ||
| pub fn new(mut s: String) -> Self { | ||
| s.shrink_to_fit(); | ||
| //Self { full: Cow::from(s), lines: OnceLock::new() } |
| } | ||
|
|
||
| pub fn new_static(s: &'static str) -> Self { | ||
| //Self { full: Cow::from(s), lines: OnceLock::new() } |
|
|
||
| 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. |
There was a problem hiding this comment.
"If the fileset has not been finalized" is no longer accurate
There was a problem hiding this comment.
Also, "gets a reference" was a bit confusing because I read it as "receives" but you meant "fetches"
| } | ||
| } | ||
|
|
||
| /// # Panics |
| 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(); |
There was a problem hiding this comment.
Is the &** still needed? You added a From for the Arc
| } | ||
|
|
||
| /// # Panics | ||
| /// Will panic if a lock on self.used can not be obtained |
There was a problem hiding this comment.
Comment is inaccurate :) See above
| 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()) |
There was a problem hiding this comment.
Calling entry.path().to_path_buf() twice here
| 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> { |
There was a problem hiding this comment.
I guess this function can be removed and the callers just call parse_pdx
|
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.) |
2 participants
Goals