Make CodeMap and FileMap thread-safe #48904
Conversation
rust-highfive
added
the
S-waiting-on-review
Zoxc
force-pushed
the
code-and-file-maps
branch
from
833cac3
to
878ac47
Compare
There was a problem hiding this comment.
Thanks for the PR, @Zoxc! The code looks correct to me. What we really should do though is make FileMap immutable. Except for the external_src business, a FileMap should only ever be modified during parsing. Maybe we can refactor things so that the parser works with a &mut FileMap?
src/libsyntax/codemap.rs
Outdated
| // This is used to apply the file path remapping as specified via | ||
| // --remap-path-prefix to all FileMaps allocated within this CodeMap. | ||
| path_mapping: FilePathMapping, | ||
| stable_id_to_filemap: RefCell<FxHashMap<StableFilemapId, Lrc<FileMap>>>, | ||
| stable_id_to_filemap: Lock<FxHashMap<StableFilemapId, Lrc<FileMap>>>, |
There was a problem hiding this comment.
I think it would make sense to put stable_id_to_filemap into the same Lock as files since they should be modified atomically.
There was a problem hiding this comment.
We could probably also remove files altogether since stable_id_to_filemap seems to contain the same information?
There was a problem hiding this comment.
We address FileMaps by their index in the files array quite a bit. For now, I would just keep both data structures.
| /// Width of characters that are not narrow in the source code | ||
| pub non_narrow_chars: RefCell<Vec<NonNarrowChar>>, | ||
| pub non_narrow_chars: Lock<Vec<NonNarrowChar>>, |
There was a problem hiding this comment.
lines, multibyte_chars, and non_narrow_chars should probably be within the same Lock too.
There was a problem hiding this comment.
These fields aren't atomically updated, but incrementally updated during parsing. So they aren't really thread-safe until after parsing. We should probably split them out of FileMap and atomically update them after parsing, or pass &mut FileMap to the parser, so no one can observe them changing. We could also use compute them lazily with a query. That may be a performance win if they are unused when no diagnostics are emitted.
There was a problem hiding this comment.
We could also use compute them lazily with a query. That may be a performance win if they are unused when no diagnostics are emitted.
I think they are used almost always because we need them for debuginfo and panic messages.
I would like FileMap to be immutable if possible. The parser could keep it's own mutable version and only register it with the CodeMap when it is finished (at which point the codemap could apply the relevant offsets), or the parser could reserve the address range beforehand if that's much more performant.
|
@Zoxc, will you be looking into making |
|
I'd prefer to do that in a separate PR. I added it to my list of things to fix. |
|
OK. The issue with |
|
I added a commit which addresses that. |
Zoxc
force-pushed
the
code-and-file-maps
branch
from
0b62853
to
65b4990
Compare
|
@bors r+ |
|
📌 Commit 65b4990 has been approved by |
bors
added
S-waiting-on-bors
Make CodeMap and FileMap thread-safe r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister