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

Optimize CcInfo computation #4020

Merged
merged 2 commits into from
Aug 16, 2024
Merged

Conversation

dzbarsky
Copy link
Contributor

@dzbarsky dzbarsky commented Aug 9, 2024

This shows up around 4% of buildbuddy analysis phase
image

After this change, it goes away.

It seems like Bazel is able to make this lazy, which is good since it's only used for building LINKMODE_C_ARCHIVE/LINKMODE_C_SHARED which is fairly rare.

What type of PR is this?
Performance improvement

What does this PR do? Why is it needed?
Avoid computing CcInfo for every intermediate library when it's not needed

Which issues(s) does this PR fix?

Fixes #

Other notes for review

@dzbarsky dzbarsky force-pushed the zbarsky/go-cc-info branch 3 times, most recently from b87d329 to 9171aea Compare August 9, 2024 00:47
@dzbarsky dzbarsky changed the title Optimize CcInfo computation WIP: Optimize CcInfo computation Aug 9, 2024
@dzbarsky dzbarsky changed the title WIP: Optimize CcInfo computation Optimize CcInfo computation Aug 14, 2024
@dzbarsky dzbarsky force-pushed the zbarsky/go-cc-info branch 3 times, most recently from 90b4e1c to bfec462 Compare August 14, 2024 17:33
Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

I'm surprised to see that this reduces CPU usage as I would have expected the aspect to be evaluated eagerly. Could you check the memory implications of having a new aspect?

go/private/rules/binary.bzl Outdated Show resolved Hide resolved
go/private/rules/binary.bzl Outdated Show resolved Hide resolved
@dzbarsky
Copy link
Contributor Author

I'm surprised to see that this reduces CPU usage as I would have expected the aspect to be evaluated eagerly. Could you check the memory implications of having a new aspect?

I did as well, but I guess it must be lazy somehow? (And if it's not, I think we have a path to make it so; the go_binary macro already splits to 2 rules for executable vs non-executable, and we only need the aspect on the non-executable one)

I'll poke at memory usage as well

@dzbarsky
Copy link
Contributor Author

I'm surprised to see that this reduces CPU usage as I would have expected the aspect to be evaluated eagerly. Could you check the memory implications of having a new aspect?

I did as well, but I guess it must be lazy somehow? (And if it's not, I think we have a path to make it so; the go_binary macro already splits to 2 rules for executable vs non-executable, and we only need the aspect on the non-executable one)

I'll poke at memory usage as well

@fmeum so I tried bazel shutdown && bazel build --nobuild //... && bazel info used-heap-size-after-gc on buildbuddy repo. I saw 215-216MB as baseline, and 220MB with this PR. I made the aspect only apply to non-executable binary and now it is down to 213-214MB, so it's actually an improvement :)

Copy link
Member

@fmeum fmeum left a comment

Choose a reason for hiding this comment

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

Great job!

@fmeum fmeum enabled auto-merge (squash) August 14, 2024 21:19
auto-merge was automatically disabled August 14, 2024 22:49

Head branch was pushed to by a user without write access

@dzbarsky dzbarsky force-pushed the zbarsky/go-cc-info branch 3 times, most recently from 8206e04 to 2328948 Compare August 15, 2024 15:37
@fmeum fmeum enabled auto-merge (squash) August 16, 2024 06:33
@fmeum fmeum merged commit a01ba7c into bazel-contrib:master Aug 16, 2024
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants