lookup exported symbols only when needed.

[andjo403]

reduces the time to compile small file with no optimization by half.

[rust-highfive]

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @nikomatsakis (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

[andjo403]

half the time was only for an empty file due to I measured wrong (had --crate-type lib for a file with only an main function ) but hay big win for #43300 :) for other files the translation time go down 10ms so 3-4%

[Mark-Simulacrum]

Could you make the exported_symbols map be an Option so that it's very clear if in the future this optimization no longer makes sense we don't end up checking an empty map and not realizing it.

Otherwise I feel pretty good about this!

r? @Mark-Simulacrum

[andjo403]

@Mark-Simulacrum shall I split it up in two optional variables then one for local create and other? or shall I only have one if with lto or not? And what shall I do in lto.rs when there is none in the variable?

is it possible to move this to the use in lto.rs my feeling was that it was not wanted/possible.

will try to fix this after work today

[michaelwoerister]

One field for the whole list is fine. In lto.rs you can call Option::expect() so that the compiler ICEs if we have a None value. Crashing is fine, since there should never be a None value if the conditions for filling the list are set up correctly.

[michaelwoerister]

Could you turn this whole block into a match tcx.sess.lto() { ... } instead of two nested ifs? It's a bit more verbose but it makes it easier to read.

[michaelwoerister]

Great find, btw!

[andjo403]

I'm not sure this PR was a good idea.
yes I get some improvements when i compile with "--emit=dep-info,metadata" but when linking is done it takes more time to compile.
have done the tested with the random small file: /src/test/run-pass/ctfe/assoc-const.rs.
from time-passes i get before this PR:
time: 0.062; rss: 118MB translation
time: 0.139; rss: 122MB linking
and with this PR:
time: 0.053; rss: 114MB translation
time: 0.189; rss: 119MB linking
so even if there is a gains in translation there is a bigger lose in the linking.
so do not know if this PR shall be continued.

also I'm to bad at rust to understand how to update the code in lto.rs if I change to a Arc<Option> when I try to get the value with the Option::expect() I get a compile error and I do not understand how to solve it.
125 | let exported_symbols = cgcx.exported_symbols.expect("needs exported symbols"); | ^^^^^^^^^^^^^^^^^^^^^ cannot move out of borrowed content

[Mark-Simulacrum]

This should have no effect on linking time, though perhaps that incorporates parts of Rust code too. I'm not sure. I don't see why this would be a net loss.

With regards to the option, you probably want something like cgcs.exported_symbols.as_ref().expect("...") or maybe as_mut() if we mutate them, but I don't think we do.

[andjo403]

have already tried with as_ref and get the same error :(
about the linktime the only thing that I can think of is that some queries is done in some other order.
When I was looking at the profile logs it was the symbol_name query that was the main time consumer of the export_symbol query as I was understanding it.

[andjo403]

got it to compile now have no idea why :)
let exported_symbols = (*cgcx.exported_symbols).as_ref().expect("needs exported symbols");

[Mark-Simulacrum]

Ah, the as_ref() call was being called on the Arc instead of the Option, so deref-ing the option first solved the problem.

[Mark-Simulacrum]

It'd actually be nice to flip the Arc and the Option since right now we're still allocating space for the Some variant even when we don't use that space. Very minor optimization, but seems like a trivial change that we should make -- if only for code clarity.

[andjo403]

started with that order of Arc and the Option but when I did not get it to compile I changed thanks for the comments.

[michaelwoerister]

yes I get some improvements when i compile with "--emit=dep-info,metadata" but when linking is done it takes more time to compile.

Linking needs the list of exported symbols too, so it will be computed there instead of earlier. However, the overall time should be the same in that case, while can skip computing the list in cases without linking. It would be great if you could verify this though (that the overall build time is roughly the same for builds that do linking).

[andjo403]

the time-passes listed abow is what differs between the runs so when linking I see a 40ms longer compile time with this PR.
so the question is why the query of exported symbols take more time in the linking pass.
will try to figure out why I get a longer link time during the weekend.

[andjo403]

the function that takes longer time is this

rust/src/librustc_trans/back/link.rs

Line 800 in 58a8e0c

fnexec_linker(sess:&Session,cmd:&mutCommand,tmpdir:&Path)

but I do not see how this PR change any thing in that function very strange.

[michaelwoerister]

That's weird. Let's test it perf.rust-lang.org to see if the difference is visible there too.

@bors try

[bors]

⌛ Trying commit 7041ef3 with merge 682bc88...

[bors]

☀️ Test successful - status-travis
State: approved= try=True

[michaelwoerister]

@Mark-Simulacrum, could you do a perf run please?

[Mark-Simulacrum]

Must've forgot to leave a comment that I queued it. http://perf.rust-lang.org/compare.html?start=27a046e9338fb0455c33b13e8fe28da78212dedc&end=682bc88006582c2e549d0c5b26a1f41a682f2a18&stat=instructions%3Au (done). Looks fairly good overall.

[andjo403]

have tested om my other computer and can not see any degradation of the compile time when linking.
In the perf run I have no idea what the problem whit style-servo-opt is.

[goffrie]

@andjo403 "style-servo-opt clean incremental" is flaky so you can ignore it (check out the front page http://perf.rust-lang.org/). So, nice wins all around :)

[Mark-Simulacrum]

r? @michaelwoerister

[michaelwoerister]

Wow, surprisingly good even! I guess with this we can skip computing upstream symbol names for rlibs entirely. Awesome find, @andjo403!

@bors r+

[bors]

📌 Commit 7041ef3 has been approved by michaelwoerister