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.

Sign up for GitHub

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

Jump to bottom

Turn nailgun initialization message down to debug #21865

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Turn nailgun initialization message down to debug #21865

wants to merge 2 commits into from

Conversation

huonw
Copy link
Contributor

@huonw huonw commented Jan 22, 2025 •
edited
Loading

I think the intention with these log lines is logging expensive operations. However, the NailgunPool::new operation itself isn't expensive, and so doesn't need to be logged (sometimes twice) on every start-up.

As a fairly-expert Pants user, I'm not sure what information one should get from knowing that there's potentially 16 processes managed by nailgun, and other less-expert users likely have even less use for it!

Fixes #21863 , revisits #20946 (comment) (particularly the last paragraph).

@huonw huonw added the release-notes:not-required PR doesn't require mention in release notes label Jan 22, 2025
@huonw huonw marked this pull request as ready for review January 22, 2025 03:09
@huonw huonw requested review from kaos, benjyw and alonsodomin January 22, 2025 03:11
benjyw
benjyw approved these changes Jan 22, 2025
View reviewed changes
Copy link
Contributor

@benjyw benjyw left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, this is good, since those messages give the impression that the nailgun startup is expensive, but actually it's the solver that is taking the time that those messages seem to be hanging on...

@jgranstrom
Copy link
Contributor

jgranstrom commented Jan 22, 2025 •
edited
Loading

It is maybe a special case, but having nailgun enabled or not can affect hermeticity of compiled output #21146. Another time it has been helpful was to indicate misconfigured memory_usage parameters. Not saying it should stay, just that there has been cases where it was actually nice-to-have.

Edit: I agree it's a bit weird in between those logs, especially without a "Nailgun pool initialized".

@huonw
Copy link
Contributor Author

huonw commented Jan 23, 2025

It is maybe a special case, but having nailgun enabled or not can affect hermeticity of compiled output #21146.

Ah, that is an unfortunate bug, but seems somewhat suboptimal to be noisy to help flag one bug in a tool. 🤔

Another time it has been helpful was to indicate misconfigured memory_usage parameters

Ah, I'm surprised by this, so I'd love more info. Do you happen to remember more about how this log line helped with that?

@jgranstrom
Copy link
Contributor

Ah, that is an unfortunate bug, but seems somewhat suboptimal to be noisy to help flag one bug in a tool. 🤔

Yes that’s fair, it just came to mind on the nailgun topic. (I actually think nailgun should even be considered to be dropped, since it’s deprecated also)

Ah, I'm surprised by this, so I'd love more info. Do you happen to remember more about how this log line helped with that?

Sure! It was a misconfiguration of process_per_child_memory_usage + process_total_child_memory_usage that was noticed when comparing log outputs. In this case it turned the pool size into 1. This log output was, (by proxy), what made it clear that the pool size was off

@huonw
Copy link
Contributor Author

huonw commented Jan 23, 2025

I actually think nailgun should even be considered to be dropped, since it’s deprecated also

Ah, I see. That prompts me to think of another way to tackle the logging-is-usually-irrelevant would be to phase out the usage of Nailgun, e.g. turn https://www.pantsbuild.org/prerelease/reference/global-options#process_execution_local_enable_nailgun from true to false by default.

#20603 looks related to this too.

Sure! It was a misconfiguration of process_per_child_memory_usage + process_total_child_memory_usage that was noticed when comparing log outputs. In this case it turned the pool size into 1. This log output was, (by proxy), what made it clear that the pool size was off

Ah, yes, I see.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@benjyw benjyw benjyw approved these changes

@kaos kaos Awaiting requested review from kaos

@alonsodomin alonsodomin Awaiting requested review from alonsodomin

Assignees
No one assigned
Labels
release-notes:not-required PR doesn't require mention in release notes
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Starting pants shows useless "Initializing Nailgun pool ..." log lines
3 participants
@huonw @jgranstrom @benjyw