-
Notifications
You must be signed in to change notification settings - Fork 605
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
fix(profilecli): disable aggregate-callees
in go-pgo query by default
#3638
base: main
Are you sure you want to change the base?
Conversation
…update the related prompt messages.
|
Hi @JansonLv, thanks for the contribution! Indeed, I believe a clarification could improve UX. The However, I have a couple of questions:
I'd like to learn more about the case:
For the context: the |
@kolesnikovae Hi, thank you for your reply. I am using version go1.22.7. |
Thank you @JansonLv! I'll double check that our
|
aggregate-callees
in go-pgo query by default
aggregate-callees
in go-pgo query by defaultaggregate-callees
in go-pgo query by default
@kolesnikovae thanks,Please check if this is the file you are looking for. |
hi,@kolesnikovae Sorry, that was generated by another project; you can review the information from these two files, and I have modified the trimstrings code, processing only part of the information. |
Thank you so much for providing the samples! Profiles look good, however they contain surprisingly few samples ~800 and ~1000 samples correspondingly (16.64s of CPU time in both). Idea of UPD: Same for the second pair: 756/1223/17.45s. I'd suggest querying a longer period of time. I'm wondering how you've measured the impact of PGO – is it possible to check the compiler logs? UPD2: correct command:
|
@kolesnikovae I obtained this through stress testing the service and did not use any related Go commands. Thank you for your guidance. |
Thank you for sharing this! I'd like to figure out why aggregation affects PGO results, because this is really helpful feature and I wouldn't like disabling it by default. In the log I see use of CGO and lots of
This will tell us how many optimisations Go compiler made (very roughy), and estimate impact of the PGO (not the app performance). As for load/stress tests – as far as I understand from the log, it might involve IO (message broker), which would very likely dominate the result. Usually, the expected improvement (reduce in CPU consumption) is within 2-5%. Also, please note that PGO won't help with C code. |
hi,@kolesnikovae I'm so sorry to keep you waiting for so long. It took me some time to set up the relevant environment locally.
There is indeed a problem, which is why I didn't add the required tag for github.com/confluentinc/confluent-kafka-go/v2/kafka." in the end, the result is still 0, even though I used the one obtained through the I think we can keep the aggregation for now, as it will take some time to investigate this issue. Enjoy your weekend ./main.go:6:13: PGO devirtualize considering call cmd.Execute() |
Hi @JansonLv, no worries at all – I'm here to help. Thank you for sharing this! Please let me know if I can help you in any way. In the meantime, I believe that clarifying the CLI help message and documentation (in |
Hi,@kolesnikovae, I'm very honored to contribute to Grafana and improve the user experience. I will make the necessary modifications and commit them as soon as possible. I'm also glad to have your help. |
Hi, @kolesnikovae, I have submitted the code, but I am uncertain whether the last sentence ‘Try both options to see which gives better for your PGO’ is suitable. I hope to get your opinion on this. Wish you a pleasant weekend |
In the default configuration related to
profilecli query go-pgo
, the exported PGO file does not optimize or improve performance. Using--no-aggregate-callees
results in a performance boost, but the prompt for theaggregate-callees
parameter is not user-friendly and may mislead users into thinking they should use--aggregate-callees=false
. Therefore, I suggest changing the default value of aggregate-callees to false and adding relevant prompts.