Use the object crate for metadata reading #83640
Conversation
|
Some changes occured to rustc_codegen_cranelift cc @bjorn3 |
|
(rust-highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
rustbot
added
the
S-waiting-on-perf
|
⌛ Trying commit aeee8d87bffdd7370b2c2bf149bf1bfaf7bc18e0 with merge 652060f31a2669f8467ef860c7b663be75b0bd44... |
This comment has been minimized.
This comment has been minimized.
|
@bors r- |
bors
added
S-waiting-on-author
bjorn3
force-pushed
the
shared_metadata_reader
branch
from
aeee8d8 to
d2b63d8
Compare
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
|
⌛ Trying commit d2b63d8cd784c90b2c96853d5acea0d6c8ab9976 with merge a7a4d9f098502325534a9e8e22b59dcec45582bc... |
This comment has been minimized.
This comment has been minimized.
bjorn3
force-pushed
the
shared_metadata_reader
branch
from
d2b63d8 to
570f9a0
Compare
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
This comment has been minimized.
|
@bors try @rust-timer queue |
|
Awaiting bors try build completion. @rustbot label: +S-waiting-on-perf |
|
⌛ Trying commit 29f195cc7bb5fbcaa7abfab7a1a9fff63ebcbcb0 with merge dfb3f56d49b7f86c1163b25c8f7d06ca511df6e1... |
|
☀️ Try build successful - checks-actions |
|
Queued dfb3f56d49b7f86c1163b25c8f7d06ca511df6e1 with parent 40334da, future comparison URL. |
The version 1 resolver unifies enabled features across the whole workspace. This includes libstd which isn't allowed to depend on wasmparser.
bjorn3
force-pushed
the
shared_metadata_reader
branch
from
9865eb5 to
802fe17
Compare
This comment has been minimized.
This comment has been minimized.
|
Spurious error. Re-running PR checks. |
| let archive = object::read::archive::ArchiveFile::parse(&*data) | ||
| .map_err(|e| format!("{:?}", e))?; | ||
|
|
||
| for entry_result in archive.members() { |
There was a problem hiding this comment.
So, I think this (and use of mmap) may be one plausible reason for the slight performance regression. If my memory of the ar format serves me well, obtaining the list of members in an ar has to process the file effectively as if it was a ump list. I suspect that such a read pattern may be a pathological for mmap based I/O: kernel would try loading more data (page?) into memory only for us to inspect the file name and length before we jump to the next entry(-ies), discarding the rest of the data that kernel spent time loading in.
Without digging into LLVM's ArchiveRO implementation I can imagine that more precise reads could be more effective here.
There was a problem hiding this comment.
ArchiveRO::open also uses mmap for header reading I think:
MemoryBufferRef doesn't export any method allowing read calls on the mapped file.
|
The implementation is very reasonable and a huge code quality improvement over the LLVM based version IMO. I'm especially happy with the unification of metadata reading code between backends. The largest regressions introduce a instruction count hit of 0.7% in benchmarks in-line with those I'd expect in terms of the workflows that would be affected by this change most (check, debug). While the hit is not trivially ignorable, its also small enough, I think, that the maintainability improvements would justify it. And it sounds like there may be a number of low-hanging fruit in archive parsing implementation too. |
|
There's a segfault in #84449 and what I think is memory corruption (probably the same thing), and I would personally love to not have to track it down to some weird interaction with the LLVM C API here. I suspect it will "magically go away" if this were all Rust-based! |
|
Note that I and @philipc found a couple of obvious places to optimize the archive reading code. Some of that has landed and will eventually make its way over to rustc during natural passage of time. Some of that would need changes on the rustc side AFAICT. AFAICT the code with |
I am not sure what you are referring to. |
|
@bors r+ Thanks.
I was referring to the experiment made in this branch (linked to by one of the comments I linked above), and in particular this commit. But its fine anyway. |
|
📌 Commit 6381aaf has been approved by |
bors
added
S-waiting-on-bors
|
☀️ Test successful - checks-actions |
Use the object crate for metadata reading This allows sharing the metadata reader between cg_llvm, cg_clif and other codegen backends. This is not currently useful for rlib reading with cg_spirv ([rust-gpu](https://github.com/EmbarkStudios/rust-gpu/)) as it uses tar rather than ar as .rlib format, but it is useful for dylib reading required for loading proc macros. (cc `@eddyb)` The object crate is already trusted as dependency of libstd through backtrace. As far as I know it supports reading all object file formats used by targets for which we support rust dylibs with crate metadata, but I am not certain. If this happens to not be the case, I could keep using LLVM for reading dylib metadata. Marked as WIP for a perf run and as it is based on rust-lang#83637.
This allows sharing the metadata reader between cg_llvm, cg_clif and other codegen backends.
This is not currently useful for rlib reading with cg_spirv (rust-gpu) as it uses tar rather than ar as .rlib format, but it is useful for dylib reading required for loading proc macros. (cc @eddyb)
The object crate is already trusted as dependency of libstd through backtrace. As far as I know it supports reading all object file formats used by targets for which we support rust dylibs with crate metadata, but I am not certain. If this happens to not be the case, I could keep using LLVM for reading dylib metadata.
Marked as WIP for a perf run and as it is based on #83637.