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

Collection of minor tweaks to get approx. 10-15% compile time #4245

Merged
merged 1 commit into from
Aug 23, 2024

Conversation

chandlerc
Copy link
Contributor

Most of these are about enabling inlining, in a couple of cases moving code to a header and throughout switching to CARBON_DCHECK. The code size of CARBON_CHECK seems to make inliing quite unreliable. I'm going to think about whether there are ways to improve this, but a reasonably small number of these seem worth switching for now to get some compile time savings.

Also moves VLOG out of the hot path which helps a bit as well.

All combined, this net a bit over 10%, although it varies a bit exactly how much. We're now pretty consistently over 800k lines/second for check in the compilation benchmark for files >=4k lines, which makes me happy. That's remarkably close to our original target.

Not really planning to keep optimizing here, just was glancing at the profile and many of these stood out to me and were easy to fix.

Most of these are about enabling inlining, in a couple of cases moving
code to a header and throughout switching to `CARBON_DCHECK`. The
code size of `CARBON_CHECK` seems to make inliing quite unreliable. I'm
going to think about whether there are ways to improve this, but
a reasonably small number of these seem worth switching for now to get
some compile time savings.

Also moves VLOG out of the hot path which helps a bit as well.

All combined, this net a bit over 10%, although it varies a bit exactly
how much. We're now pretty consistently over 800k lines/second for check
in the compilation benchmark for files >=4k lines, which makes me happy.
That's remarkably close to our original target.

Not really planning to keep optimizing here, just was glancing at the
profile and many of these stood out to me and were easy to fix.
(vlog_stream_ == nullptr) ? (void)0 \
: CARBON_VLOG_INTERNAL_STREAM(vlog_stream_)
#define CARBON_VLOG() \
__builtin_expect(vlog_stream_ == nullptr, true) \
Copy link
Contributor

Choose a reason for hiding this comment

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

Should CARBON_CHECK have a similar __builtin_expect((__VA_ARGS__), false) ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The noreturn provides some help there, but yeah, we can probably do more. Will send another PR.

@jonmeow jonmeow added this pull request to the merge queue Aug 23, 2024
Merged via the queue into carbon-language:trunk with commit 5d0ec91 Aug 23, 2024
10 checks passed
dwblaikie pushed a commit to dwblaikie/carbon-lang that referenced this pull request Aug 23, 2024
…-language#4245)

Most of these are about enabling inlining, in a couple of cases moving
code to a header and throughout switching to `CARBON_DCHECK`. The code
size of `CARBON_CHECK` seems to make inliing quite unreliable. I'm going
to think about whether there are ways to improve this, but a reasonably
small number of these seem worth switching for now to get some compile
time savings.

Also moves VLOG out of the hot path which helps a bit as well.

All combined, this net a bit over 10%, although it varies a bit exactly
how much. We're now pretty consistently over 800k lines/second for check
in the compilation benchmark for files >=4k lines, which makes me happy.
That's remarkably close to our original target.

Not really planning to keep optimizing here, just was glancing at the
profile and many of these stood out to me and were easy to fix.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants