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

Reduce amount of filesystem probing rustc has to do when looking for rlibs #46816

Closed
alexcrichton opened this issue Dec 18, 2017 · 13 comments
Closed
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@alexcrichton
Copy link
Member

Currently rustc has a requirement that whenever it compiles a crate that it understands the entire crate graph of all dependencies. In other words, when you compile a crate like cargo the compiler will have to load all dependencies (including dependencies of dependencies). This means that rustc has to actually find and locate the rlib artifacts on the filesystem.

For immediate dependencies (those specified via extern crate) the compiler has --extern arguments (passed by Cargo) to guard it. For dependencies of dependencies Cargo knows where the files are but doesn't tell rustc. Instead rustc is left to its own devices to actually find these rlibs.

This probing logic is currently implemented like so:

  • Look at all files in the search paths of the comipler (passed via -L and also the sysroot).
  • When looking for crate foo, filter files which start with libfoo and end with .rlib
  • Given this set of candidates, look at the metadata inside of all of them. Filter these candidates by the "hash" we're loading
  • If only one candidate remains, link to it. If more than one is here, bail out and generate an error.

The "hash" is listed for all dependencies of dependencies because the compiler will record this information in the metadata of a compiled crate. For example of we have a dependency chain that looks like cargo depends on tar which depends on libc, then we'll get loading behavior like so:

  • The tar crate is found via --extern on the command line.
  • When processing tar rustc needs to load the libc crate. The metadata for tar, however, lists the hash of the libc crate it was originally compiled against.
  • The compiler will go through the process above to find all libc candidates and then use the hash listed in tar's metadata to find the one exact crate we're looking for.

Ok so with all that information, let's talk about downsides! One of the drawbacks of this logic is pretty obvious when we take a look at the serde crate. Let's say that rustc is looking for the serde dependency. This means it'll also look at crates like serde_derive, serde_json, serde_yaml, etc (see the trend?). While not a huge perf problem today, this definitely has the chance to become a larger problem.

More worrisome, however, is that the Gecko folks are trying to use the Tup build system and when adding rustc support this causes lots of problems. Tup as a build system tracks what files are accessed as part of a build process (aka it's tracking what files rustc itself is accessing). To Tup it looks like rustc is depending on all these files! Rustc in reality is rejecting many of these files and not actually using them, but to Tup they look like false dependencies.

In order to solve this problem, let's get rustc to probe for less files at build time! The logic here of prefix/suffix matching is very old I believe and in this day and age I'm not actually sure if it's buying us much. Instead I think it may be best to switch to a precise match solution where we look for an exact filename on the filesystem instead of a set of filetimes.

All Rust crates compiled through Cargo (and almost all Rust crates in general) are compiled with -C extra-filename which is where those funny hashes get inserted into rlib filenames. These extra hashes are not currently encoded into a crate's metadata, but they'd be able to point rustc to the exact file that it needs!

I think that we'll want to start encoding the -C extra-filename argument into a crate's metadata so that way transitive dependencies can copy this information and then when rustc loads it it knows exactly what to look for.

Given our cargo example above, the libc loading step would now look like:

  • The tar crate's metadata says it depends on libc which was found with the extra filename as $extra_filename. The compiler then looks for exactly libc${extra_filename}.rlib and doesn't look at extraneous files.

This I believe should solve the Tup problems and also head off any possible inefficiences in crate loading. I'm also more than willing to help out mentor the implementation of this if anyone's interested, just let me know!

cc @luser
cc @mshal

@alexcrichton alexcrichton added the E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. label Dec 18, 2017
@hgallagher1993
Copy link
Contributor

I'll take this on if it's not massively urgent because Christmas week and maybe the week after will probably be pretty busy for me.

@alexcrichton
Copy link
Member Author

@hgallagher1993 awesome! Let me know if you need anything!

@luser
Copy link
Contributor

luser commented Dec 19, 2017

I doubt anyone is going to be relying on anything for the next few weeks. :) If you get a working patch and want someone to test it, drop us a line and @mshal would likely be interested!

@mshal
Copy link

mshal commented Dec 20, 2017

Yep, I'd definitely be interested in helping to test any prototypes you have. Please reach out when you think you have something working! Thanks for taking this on.

@hgallagher1993
Copy link
Contributor

First of all sorry for the (very) late update, I have actually had a look at this and I think I've a good idea about what needs to be done but maybe not entirely the where or how.

I think that we'll want to start encoding the -C extra-filename argument into a crate's metadata

This is one thing in particular I'm not really sure of, just as a start where exactly would the code for encoding the extra filename argument need to go?

@alexcrichton
Copy link
Member Author

@hgallagher1993 I think the encoding would be somewhere around here after updating this struct

@kennytm kennytm added C-enhancement Category: An issue proposing an enhancement or a PR with one. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 6, 2018
@mshal
Copy link

mshal commented Feb 20, 2018

@hgallagher1993 - have you had any luck here? Do you need someone else to help take a look?

@hgallagher1993
Copy link
Contributor

@mshal There was just over a week where I really didn't get the chance to do anything with this but the last 2 days after work I've been looking at this and have made a bit of progress but I've nothing ready to test just yet.

@hgallagher1993
Copy link
Contributor

Hi so again sorry for the delay the project I was working on for the last few months at my job went for User acceptance testing last Thursday and things got....busy in the build up to it to say the least, but anyway how would I go about testing this locally just for when I have something to test (hopefully the next few days)?

@chmanchester
Copy link
Contributor

Hi @hgallagher1993, any progress so far? I was thinking of taking a stab at this as it's blocking some Firefox build system work.

In any case, thank you for taking a look!

@hgallagher1993
Copy link
Contributor

@chmanchester Progress hasn't been good, quite frankly. I've spent a good bit of time looking at it the last week or 2 but I haven't got very far at all and was actually intending on coming back here and asking for a bit more direction but if it's blocking the build system it'd be better if you (or anyone) looked at it because I've no idea how long it might take me to finish it.

@chmanchester
Copy link
Contributor

Ok, thanks!

I was able to make some progress here. @mshal, I have a branch at https://github.com/chmanchester/rust/tree/probing_fix that should be worth testing against a tup build of m-c. As discussed, I was able to eliminate a significant amount of metadata reading based on the approach suggested here, but there may be some more cases for us to track down.

@mshal
Copy link

mshal commented Mar 19, 2018

@chmanchester, I gave your branch a whirl today. It looks awesome! All the extraneous file issues I had before were gone. Specifically,

  1. libc & librand are no longer read by all rustc invocations
  2. Multiple versions of eg: libbitflags are not read when only a single version is required
  3. Host/target versions of libraries are kept separate
  4. Random extra libraries that match a prefix (eg: libcubeb_sys matching libc*) are not used.

So I'm definitely happy with this. It makes the logic to go from 'cargo --build-plan' to a Tupfile much simpler, and incremental builds now work properly. I was actually able to get all of the rustc invocations that are output by --build-plan to complete successfully on a recent mozilla-central tree. Well done!

chmanchester added a commit to chmanchester/rust that referenced this issue Mar 29, 2018
alexcrichton added a commit to alexcrichton/rust that referenced this issue Apr 5, 2018
…chton

Take the original extra-filename passed to a crate into account when resolving it as a dependency

resolving it as a dependency.

Fixes rust-lang#46816
kennytm added a commit to kennytm/rust that referenced this issue Apr 5, 2018
…chton

Take the original extra-filename passed to a crate into account when resolving it as a dependency

resolving it as a dependency.

Fixes rust-lang#46816
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-enhancement Category: An issue proposing an enhancement or a PR with one. E-mentor Call for participation: This issue has a mentor. Use #t-compiler/help on Zulip for discussion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

6 participants