Ensure lookup_source_file_idx has a sorted list of source files to use
#115328
Conversation
|
(rustbot has picked a reviewer for you, use r? to override) |
rustbot
added
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
|
||
| files.source_files.push(source_file.clone()); | ||
| files.stable_id_to_source_file.insert(file_id, source_file.clone()); | ||
| self.insert_source_file(&source_file, file_id); |
There was a problem hiding this comment.
I was wondering if we could keep the same behaviour by locking files between allocate_address_space and the insertion. For instance:
allocate_address_spacelocksfiles, returns both the last file's end pos and the lock guard;- this line finalises the push to
filesand releases the lock.
This would get rid of used_address_space at the same time.
There was a problem hiding this comment.
That would require holding the lock during SourceFile::new, which is expensive and rather complex.
There was a problem hiding this comment.
Also this moves lookup_source_file_idx from RwLock to Lock which is probably better performance wise. Not sure how hot it is for regular compilation though.
There was a problem hiding this comment.
Looking at SourceFile::new, I understand that it indexes all the lines in the file using the result of allocate_address_space.
Would using relative indexing inside the file avoid this dependency:
- all indices in-file are done wrt. the beginning of the file;
- the contents of
SourceFileget independent of the absolute byte pos; - we can assign the start byte pos after
SourceFile::newreturns.
There was a problem hiding this comment.
I think that would work, but I'm not looking to change a lot of source related function now.
SourceFile / SourceMap is very much due a refactoring to remove the mutable state, but it probably makes sense to wait until we can use the query system there.
|
☔ The latest upstream changes (presumably #115507) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Closed in favor of #115507. |
This fixes a race condition where
lookup_source_file_idxassumes that the list of source files is sorted, but that wasn't ensured with concurrent insertions of source files. This adds a new sorted source file list (sorted_files) dedicated for use inlookup_source_file_idx.