Repository navigation
Potential performance regression from new LTO effort after export changes #71248
Description
Spawned off of #71131 (comment)
PR #71131 may have injected a performance regression into incremental compilation. It certainly seemed like it did on the two try runs we did on the PR itself:
- Do not reuse post LTO products when exports change #71131 (comment)
- Do not reuse post LTO products when exports change #71131 (comment)
So this issue is to track the investigation of that potential regression. (Namely, to evaluation whether a regression truly happened; assuming it did, then what its underlying causes were, and whether any of the significant sources of regression are avoidable.)
Activity
pnkfelix commentedon Apr 17, 2020
Interesting: I instrumented the first case I was interested in ("is valid cap letter" on
regex, as noted here), and it reports that in all five of the cases where we are now not reusing the Post-LTO build product, it is because theexports_all_greenboolean is false; the set comparison test that I was worried about isn't even being run!I'll have to go back and review the original logic about the all-green flags; I will admit that when I transcribed that part of the code, it was more a blind "well surely this matters" kind of reasoning.
pnkfelix commentedon Apr 17, 2020
(My intuition is that we do need the
exports_all_greencomputation, and when it is non-green we do need to re-do LTO. After all, the basis of #69798 (comment) was that a global symbol in module A was determined to be unused in module B, and thus the global symbol was demoted to a local to A; then a later change to B injected a use of that global symbol, and so the change to the importing module (B) is significant to the compilation of A.Another, separate question is whether injecting
{ }(as the "is valid cap letter" patch does) can be made something that doesn't trigger this code to treat changed module as non-green here...)pnkfelix commentedon Apr 17, 2020
Having said that, I did prototype taking out
exports_all_green, and with it gone, the regression test for #69798 still passes...pnkfelix commentedon Apr 17, 2020
I suppose if the import map actually reflects the true usage of the symbols in the importing module, then in fact it should not be necessary for us to look at
exports_all_green.I'm willing to have a go with a PR that removes it and see how things go.
Issue rust-lang#71248: attempt to recover perf by removing `exports_a…
Issue rust-lang#71248: attempt to recover perf by removing `exports_a…
Auto merge of rust-lang#71267 - pnkfelix:issue-71248-dont-need-export…
Mark-Simulacrum commentedon Apr 29, 2020
@pnkfelix Now that #71267 has landed, I'm going to go ahead and close this but feel free to reopen if you think there's more to track here.
fenollp commentedon May 3, 2020
I've opened #71850 which seems related.