Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

upgrade ar crate so we can do less copying when trimming rlibs #790

Merged
merged 1 commit into from
Jun 11, 2020

Conversation

froydnj
Copy link
Contributor

@froydnj froydnj commented Jun 10, 2020

The current trimming of rlibs in the dist-sccache case looks like:

  1. Find the rust.metadata.bin file in the rlib (archive).
  2. Read its data.
  3. Create a new archive that will only contain said file.
  4. Copy the data into the new archive.

We can do better than this, by doing the following:

  1. Find the rust.metadata.bin file in the rlib (archive).
  2. Create a new archive that will only contain said file.
  3. Copy the data from the original rlib directly into the new archive.

We thus save a copy.

All that being said, it looks like recent versions of Rust don't
actually output rust.metadata.bin files into rlibs...unless we ought
to be only taking the lib.rmeta file that lives inside the archive.
But that would be a separate fix. (It's also great that we're seemingly
copying the rmeta file twice, once because it lives in the rlib and once
because it lives on disk as a separate file...)

@froydnj froydnj requested a review from nnethercote June 10, 2020 13:59
@froydnj
Copy link
Contributor Author

froydnj commented Jun 10, 2020

FWIW, it looks like rust-lang/rust#66235 changed the filename inside rlibs, so sccache should be updated to handle that. I'll do that in a separate PR.

@luser
Copy link
Contributor

luser commented Jun 10, 2020

This is probably not actually useful since Rust 1.38 shipped with cargo enabling pipelined compilation. The point of this code was to ship less data over the wire for distributed compilation but now cargo will use the rmeta file for non-linking compiles (the only sort sccache can distribute anyway) so it shouldn't ever be necessary to strip data out of rlibs.

@froydnj
Copy link
Contributor Author

froydnj commented Jun 10, 2020

This is probably not actually useful since Rust 1.38 shipped with cargo enabling pipelined compilation. The point of this code was to ship less data over the wire for distributed compilation but now cargo will use the rmeta file for non-linking compiles (the only sort sccache can distribute anyway) so it shouldn't ever be necessary to strip data out of rlibs.

Does that mean sccache should stop shipping the rlib entirely? That would be even better than rlib trimming!

@glandium glandium removed the request for review from nnethercote June 11, 2020 01:29
The current trimming of rlibs in the dist-sccache case looks like:

1. Find the `rust.metadata.bin` file in the rlib (archive).
2. Read its data.
3. Create a new archive that will only contain said file.
4. Copy the data into the new archive.

We can do better than this, by doing the following:

1. Find the `rust.metadata.bin` file in the rlib (archive).
2. Create a new archive that will only contain said file.
3. Copy the data from the original rlib directly into the new archive.

We thus save a copy.

All that being said, it looks like recent versions of Rust don't
actually output `rust.metadata.bin` files into rlibs...unless we ought
to be only taking the `lib.rmeta` file that lives inside the archive.
But that would be a separate fix.  (It's also great that we're seemingly
copying the rmeta file twice, once because it lives in the rlib and once
because it lives on disk as a separate file...)
@glandium glandium merged commit e3ec58d into mozilla:master Jun 11, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants