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

PGO: Disable instrumentation pre-inlining pass by default. #64095

Closed

Conversation

@michaelwoerister
Copy link
Contributor

commented Sep 2, 2019

LLVM's IR-level instrumention feature (which is used for PGO) has a pre-inlining pass. This pass performs some initial inlining and other small cheap optimizations before adding the instrumentation counters to the IR. For C++ code compiled with Clang this seems to have pretty significant benefits (see e.g. what happens if pre-inlining is turned off for C/C++ code in Firefox). For Rust code, however, this does not seem to be true.

So far, PGO seems to be rather ineffective for Rust code in general, but with pre-inlining enabled, it seems to even be detrimental to performance in most cases. E.g. when optimizing the regex benchmarks with PGO, I get the following results:

PGO Settings Speedup
pre-inlining threshold=75 (default) 0.976
pre-inlining threshold=300 0.997
pre-inlining threshold=150 1.001
pre-inlining disabled 1.009

So PGO is able eke out a ~1% speedup, if pre-inlining is disabled, but if it is enabled, the benchmarks run usually slower than without PGO. For doing benchmarks with FF, the picture is similar.

PGO's effectiveness with Rust should be investigated more deeply in any case, but for now this PR defaults to disable pre-inlining. It can be enabled on a case by case basis via -Cllvm-args, if needed.

This PR also removes the -Z disable-instrumentation-preinliner flag since this setting is already available via -C llvm-args=-disable-preinline (even on stable).

PGO: Disable instrumentation pre-inlining pass by default since Rust …
…code seems to profit from that.

Also remove the `-Z disable-instrumentation-preinliner` flag since this
setting is already available via -Cllvm-args=-disable-preinline (even
on stable).
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

commented Sep 2, 2019

r? @eddyb

(rust_highfive has picked a reviewer for you, use r? to override)

@eddyb
eddyb approved these changes Sep 3, 2019
@eddyb

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

r=me but also cc @alexcrichton

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 3, 2019

Actually, cc @rust-lang/compiler and @rust-lang/compiler-contributors -- in case somebody is interested in PGO.

@cramertj

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

@cramertj

This comment has been minimized.

Copy link
Member

commented Sep 3, 2019

Did you happen to record any binary size numbers in addition to the raw perf stats you posted above? In our own testing, we've seen C++ binaries drop 5-7% when PGO is enabled.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 6, 2019

The binaries in my test seem to be almost the same size:

no PGO:             13,205,680 bytes
PGO (default):      13,205,632 bytes
PGO (no-preinline): 13,205,656 bytes

which is kind of suspicious ...

@JohnCSimon

This comment has been minimized.

Copy link

commented Sep 14, 2019

Ping from triage

@michaelwoerister @petrhosek @cramertj @eddyb
Any update on this?

Thanks.

@michaelwoerister

This comment has been minimized.

Copy link
Contributor Author

commented Sep 16, 2019

I'm closing this for now since I've gotten interesting leads on the llvm-dev mailing list on how to further investage PGO effectiveness. Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
5 participants
You can’t perform that action at this time.