-
Notifications
You must be signed in to change notification settings - Fork 1.5k
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
Directly use TCMalloc rather than the system malloc on Linux. #4133
Conversation
This improve the toolchain's performance by about 10%. It will also allow us to leverage TCMalloc's extensions to do heap profiling and get other information about how efficiently we're using the heap. Note that currently this causes all of our builds to produce a warning due to an issue with `rules_python` and multiple modules registering python toolchains: bazelbuild/rules_python#1818 This is also only enabled on Linux as there is no support for other OSes at the moment.
Well pfft, no mac support. Hopefully fixed to be Linux-only. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LG, can you please confirm though whether there's a specific feature in the more recent tcmalloc commit that you needed?
# The registry has a snapshot, but upstream is active and not regularly marking | ||
# releases. We start with the BCR snapshot to avoid a miss and then override it | ||
# with a more recent commit. | ||
bazel_dep(name = "tcmalloc", version = "0.0.0-20240411-5ed309d", dev_dependency = True) | ||
git_override( | ||
module_name = "tcmalloc", | ||
# HEAD as of 2024-07-14. | ||
commit = "923df94c922e0cd2d0512c1662d374f63c2c0c96", | ||
remote = "https://github.com/google/tcmalloc.git", | ||
) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If there's a specific feature that you needed that's been added in the past three months, can you please note it? These extra versions are incremental work to maintain, and I'd tend to remove it on an update if there's a newer BCR version. I mean, we basically only update LLVM when there's a breaking change, and I can see similar happening here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I do understand that each of these extra versions is incremental work, FWIW.
I don't have a single, specific feature that I'm looking for here, or I would have commented about it. However, just the first page of commits only goes back to mid-June, and has at least three micro-optimizations that are likely to apply to Carbon's usage. It also has fixes to UBSan usage and other things that seem relevant to our usage. Summarizing this was what I attempted in the comment, but maybe there is some other context I can add to it that would help?
Overall, while there is some cost here, I think this is a place where it's valuable for us to track upstream fairly closely, similar to LLVM. That's at least my judgement call in this case.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, just having the notes in the PR discussion helps if I'm trying to remember for an update.
…-language#4133) This improve the toolchain's performance by about 10%. It will also allow us to leverage TCMalloc's extensions to do heap profiling and get other information about how efficiently we're using the heap. Note that currently this causes all of our builds to produce a warning due to an issue with `rules_python` and multiple modules registering python toolchains: bazelbuild/rules_python#1818 This is also only enabled on Linux as there is no support for other OSes at the moment.
This improve the toolchain's performance by about 10%.
It will also allow us to leverage TCMalloc's extensions to do heap profiling and get other information about how efficiently we're using the heap.
Note that currently this causes all of our builds to produce a warning due to an issue with
rules_python
and multiple modules registering python toolchains:bazelbuild/rules_python#1818
This is also only enabled on Linux as there is no support for other OSes
at the moment.