Make CodeMap and FileMap thread-safe#48904
Conversation
833cac3 to
878ac47
Compare
michaelwoerister
left a comment
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
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.
src/libsyntax_pos/lib.rs
Outdated
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. |
0b62853 to
65b4990
Compare
|
@bors r+ |
|
📌 Commit 65b4990 has been approved by |
Make CodeMap and FileMap thread-safe r? @michaelwoerister
|
☀️ Test successful - status-appveyor, status-travis |
r? @michaelwoerister