golang / go

The Go programming language
https://go.dev
BSD 3-Clause "New" or "Revised" License
123.02k stars 17.54k forks source link

cmd/pgopack: cache packed pgo profile during build #62400

Closed jinlin-bayarea closed 8 months ago

jinlin-bayarea commented 1 year ago

The PGO-enabled Go compilation shows the significant latency improvement of generated binary which benefits from profile-guided inlining and code specialization. However, the workflow of processing and extracting profiling data is surprisingly suboptimal in the community Go compiler. Based on the observation and instrument timing data from small benchmark and real service, we notice that the overall pprof data parsing time varies from 70% to over 95% of total time when PGO flow is enabled in the compilation. Furthermore, the existing PGO flow reads and parses the pprof file in every single package compilation and the cumulative time of this process is long and unnecessary. One-shot processing profiling data and cache information in a well-designed format for optimization transformation among all packages is critical.

To optimize the existing flow and reduce build time of multiple Go services and projects, we propose to create a standalone tool, PGO preprocessor, to extract information from collected profiling files and to cache the WeightedCallGraph in one time fashion. By adding the new tool to the Go compiler, it reduces the time of repeated profiling file parsing in current Go Compiler. The new tool is capable of all existing PGO enabled optimizations including the inlining and devirtualization in the Go compiler.

In summary, we propose adding a standalone preprocess tool to reduce the compile time when the PGO is enabled. Inputs are welcome.

cespare commented 1 year ago

For the record, speeding up the underlying work is #58102.

cespare commented 1 year ago

@jinlin-bayarea Are your .pgo files in version control, so that all builds use PGO? I know that's the Go team's recommendation (here, for example).

At my company we've only started playing around with PGO a bit but our current approach is to only build with PGO as part of production deploys, not local development/testing. So having PGO builds be a bit slower doesn't really matter much.

(We don't want to put the .pgo files in our repo because we are collecting very large profiles and we are updating them with automated processes.)

jinlin-bayarea commented 1 year ago

The pgo files are not inversion control. We have collected our pgo profiles daily and always use the most recent profile to build the service code.

prattmic commented 1 year ago

cc @aclements @cherrymui

cherrymui commented 1 year ago

Discussed briefly offline. The idea of preprocessing sounds good overall.

It would be good if you could share a prototype CL. Then we can discuss the details from there, like the exact format of the preprocessed profile. Thanks.

jinlin-bayarea commented 1 year ago

Hi Cherry. I have submitted the prototype in this CL. https://go-review.googlesource.com/c/go/+/529738. Please help review the changes. Thanks.

rsc commented 10 months ago

I would suggest calling it pgopack or something like that instead of "preprofile" since Go has lots of profiles. Otherwise this seems fine, and it should be invisible to users, so I think it can proceed with compiler/runtime team agreement instead of a proposal.

/cc @bcmills

cherrymui commented 10 months ago

Taking out of proposal as this is invisible to users using the go command. The tool will be internal (the go command will run it automatically if needed), so is the preprocessed profile format. Users are not expected to check in profiles generated by the preprocessing tool.

jinlin-bayarea commented 10 months ago

Agree with @cherrymui . For bazel build users, they need to use the preprocess command separately to retrieve the preprocessed profile.

prattmic commented 8 months ago

Tool is submitted in https://go.dev/cl/529738. It still needs integration with cmd/go. I'm going to close this issue in favor of #58102, which is effectively a duplicate of this.