Preliminary work for incremental ThinLTO. #52266
Conversation
|
r? @pnkfelix (rust_highfive has picked a reviewer for you, use r? to override) |
rust-highfive
added
the
S-waiting-on-review
This comment has been minimized.
This comment has been minimized.
|
Looks like something went wrong during rebasing... |
michaelwoerister
force-pushed
the
incr-thinlto-preliminaries
branch
from
099d438
to
fff0037
Compare
This comment has been minimized.
This comment has been minimized.
michaelwoerister
force-pushed
the
incr-thinlto-preliminaries
branch
from
fff0037
to
f6894eb
Compare
This comment has been minimized.
This comment has been minimized.
|
The job Click to expand the log.I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
|
r? @alexcrichton might be the best reviewer for this. |
src/rustllvm/PassWrapper.cpp
Outdated
| /// entry is a pointer that points to a null-termined array of module names. The | ||
| /// first entry is always the name of the *importing* module, the following | ||
| /// entries are the names of the modules it imports from. Each module name is | ||
| /// a regular C string. |
There was a problem hiding this comment.
Maybe say "regular malloc'ed C string" or something along those lines, just so someone doesn't have to scan the code for those calls to strndup ?
There was a problem hiding this comment.
Also note: In the current version, the allocations here are just leaked. I should probably fix that.
| } | ||
| } | ||
|
|
||
| pub fn save_to_file(&self, path: &Path) -> io::Result<()> { |
There was a problem hiding this comment.
This + load_to_file may be a "job for serde" if we slap a #[derive] on there, although I guess both Serde doesn't work as well as rustc-serialize not being hooked up easily to things like JSON? In that case this is probably fine-enough for now.
There was a problem hiding this comment.
I think there's still a JSON serializer in rustc-serialize but I don't really trust it. Serde would be great.
|
|
||
| impl ThinLTOImports { | ||
|
|
||
| pub fn new_empty() -> ThinLTOImports { |
There was a problem hiding this comment.
Stylistically this could also be named new like HashMap::new()
There was a problem hiding this comment.
I wanted to be as explicit as possible. But I don't mind changing it to just new().
| /// Load the ThinLTO import map from ThinLTOData. | ||
| unsafe fn from_thin_lto_data(data: *const llvm::ThinLTOData) -> ThinLTOImports { | ||
| let raw_data: *const llvm::ThinLTOModuleImports = | ||
| llvm::LLVMRustGetThinLTOModuleImports(data); |
There was a problem hiding this comment.
This feels sort of overly nasty in terms of converting a type from C++ to Rust, but then again I'm not sure of a better way to do this! In any case I do agree though that we'll want to free the raw_data here at the end of this function (probably via a dtor or something like that)
There was a problem hiding this comment.
Yeah, I don't like it either :) Suggestions on how to make this nicer are welcome.
I thought I could keep the complex data structure on the C++ and provide a C interface but that quickly seemed like a lot of infrastructure for just passing a map around.
I'll just implement a free function for the data structure. Or I could make it callback based... that would make it a bit less nasty, memory-management-wise.
|
I've updated the code loading the ThinLTO import data into |
|
@bors: r+ Nice! I had originally thought to comment that a callback and/or more structured piece of data could work here, although I thought we also run the risk of these symbol lists being huge so the FFI traffic could be a bit of a perf hit. In any case let's see how it turns out and optimize if necessary |
|
📌 Commit e045a6c has been approved by |
bors
added
S-waiting-on-bors
…=alexcrichton Preliminary work for incremental ThinLTO. Since implementing incremental ThinLTO is a bit more involved than I initially thought, I'm splitting out some of the things that already work. This PR (1) adds a way accessing some ThinLTO information in `rustc` and (2) does some cleanup around CGU/object file naming (which makes things quite a bit nicer). This is probably best reviewed one commit at a time.
|
☀️ Test successful - status-appveyor, status-travis |
|
This is a major perf regression: Lots of regressions of up to 5% for incremental compilation. Also a 9.6% regression for an nll-check build of @michaelwoerister: Is this expected? Should this be backed out? |
|
Interesting. No, that's not expected at all. I'm wondering where it comes from. It should mostly be harmless refactorings and new code that isn't called yet. I won't be available next week to investigate, unfortunately. I'm OK with backing it out in the meantime. |
|
Backing out seems best, especially given that you won't be around next week. What's the usual backout procedure for rustc? |
|
Good question, I never had to back out anything before What do we usually do, @rust-lang/release @rust-lang/compiler @rust-lang/infra? |
|
You can send a PR reverting this one. |
|
You could just |
| module_name_callback(callback_payload, | ||
| importing_module_id.c_str(), | ||
| imported_module_id.c_str()); | ||
| } |
There was a problem hiding this comment.
You could've also provided an iterator interface, like a bunch other LLVM APIs we support.
There was a problem hiding this comment.
Do you have an example?
There was a problem hiding this comment.
Searching for Iterator in src/librustc_llvm and src/rustllvm is enough. One of them is ArchiveIterator:
Lines 1687 to 1692 in 7afa0cc
| } else { | ||
| Symbol::intern(&CodegenUnit::mangle_name(FALLBACK_CODEGEN_UNIT)).as_interned_str() | ||
| } | ||
| CodegenUnit::build_cgu_name(tcx, LOCAL_CRATE, &["fallback"], Some("cgu")) |
There was a problem hiding this comment.
This may now do a bit more work than before. Ideally it would be cached (although I doubt this is the cause of the regression).
|
The regression here is really weird. For anyone willing to investigate, I'd suggest doing a perf with only f6894eb backed out - it's possible there may be a strange interaction with LLVM, not sure yet. |
|
OK, I found some time to run the In light of that I don't think reverting the whole PR is necessary. It should not be too hard to just optimize |
|
@michaelwoerister: I have a suggestion: back this PR out, do your investigation and improvements, and then re-land the adjusted patches in a new PR. This will give a much clearer picture of the overall performance impact -- we won't have to manually compare regressions from one commit and improvements from a later commit, with a bunch of unrelated commits in between. It also means that the repo won't be in this performance-degraded state while you are investigating -- which complicates performance analysis that other people are doing -- and you'll feel less rushed about landing a fix. You'll also be able to do perf runs on the full patch stack. I understand that it's something of a hassle, and will cause you more work. But getting serious about compiler performance requires getting stricter about performance regressions. A willingness to backout problematic code is how we do things in the Firefox repo, and I think it makes sense here. What do you think? |
|
@nnethercote I've opened #52422. As I said, I don't have a problem with backing it out. Being able to take more time with a fix is a good thing.
I don't quite follow the argument here. You always have to pick some stable baseline you're comparing a change against. Whether that baseline includes this PR or not shouldn't really make a difference, right? (unless you are doing something that specifically touches the same code) |
|
It won't bother most people. But I track performance over time locally, as a kind of sanity check against perf.rust-lang.org, and regressions interfere with that. (E.g. I deliberately waited until this was backed out before doing this week's run.) In case you're curious, here's what the table currently looks like. These are instruction counts, in billions, measured by Cachegrind, for Clean Debug builds. |
|
@nnethercote FWIW both |
Preliminary work for incremental ThinLTO (CGU name edition) Bring back the first half of #52266 but hopefully without the performance regression.
Preliminary work for incremental ThinLTO (CGU name edition) Bring back the first half of #52266 but hopefully without the performance regression.
Since implementing incremental ThinLTO is a bit more involved than I initially thought, I'm splitting out some of the things that already work. This PR (1) adds a way accessing some ThinLTO information in
rustcand (2) does some cleanup around CGU/object file naming (which makes things quite a bit nicer).This is probably best reviewed one commit at a time.