Use the target's real cache line size in the hist builder - #12453
Open
bryceweiner wants to merge 2 commits into
Open
Use the target's real cache line size in the hist builder#12453bryceweiner wants to merge 2 commits into
bryceweiner wants to merge 2 commits into
Conversation
`CacheManager` detects cache sizes with CPUID on x86_64 and sysfs on non-x86_64
Linux, and falls back to compiled defaults everywhere else. macOS on arm64 hits
the fallback, so XGBoost tunes its CPU histogram build against a 32KB L1 and a
1MB L2 regardless of the hardware it is running on.
Add a sysctl-based path for Apple Silicon. Two details differ from x86 and shape
the implementation:
- Cores are heterogeneous, so sizes are read per performance level and the
minimum is taken. Threads are spread over both core types, and the header
already notes that overestimating a cache costs more than underestimating it:
a block sized for a performance core's L1 would spill on an efficiency core.
- L2 is shared by every core in a cluster rather than being private per core, so
the raw `hw.perflevelN.l2cachesize` is not what a single thread can rely on.
Dividing by `cpusperl2` restores the per-thread meaning that the callers in
hist_util.cc and tree/hist/histogram.h assume. A level only contributes an L2
estimate when its sharing factor is known, since an undivided cluster size
would overestimate the budget several-fold.
The policy that reduces per-level readings to per-thread sizes is kept separate
from the sysctl query, in ReduceHeterogeneousCaches, and is compiled on every
platform rather than inside the Apple branch. Deciding what a thread can rely on
across heterogeneous cores is a judgement call, and welding it to sysctl would
have left every branch except the one the developer's machine happens to take
untested. It is now covered directly with synthetic readings: one homogeneous
level, a two-level performance/efficiency split in both orders, a level with an
unknown sharing factor, absent and non-positive readings, and a sharing factor
larger than the cluster.
That last case is why the separation was worth it. A cluster smaller than its
sharing factor divides to zero, and zero would then have been stored as though
it were a detected size, so callers would divide by it rather than fall back to
the default. The reduction discards a non-positive result.
The query returns std::optional rather than a sentinel, so there is no -1 to
keep in sync with CacheManager::kUninitCache, which is private to the class. A
sysctl value that would overflow int64 is rejected rather than wrapping.
The Apple system level cache is not exposed through sysctl, so L3 is left unset
and keeps its default of 0. Intel Macs are unaffected: they still take the
CPUID path, which is checked first.
On an M4 Max (12P+4E) this raises the detected L1d from the compiled 32KB
default to the 64KB the efficiency cores report. The derived per-thread L2 works
out to exactly the 1MB default on that part, so L1 is the only value that
changes there, but both are now read from the hardware rather than guessed.
The L1 value feeds block_size in ConstructHistSpace, which only matters on the
read_by_column path that dense data takes once the histogram outgrows the L2
budget (above roughly 200 features at the default max_bin):
usable_l1 = 0.8 * L1; space = usable_l1 - 2 * sizeof(GradientPairPrecise) * max_bin
block_size = space / (sizeof(GradientPair) + 3 * sizeof(size_t))
The size of the gain therefore depends on max_bin, since the per-column
histogram is subtracted from the L1 budget before rows get the remainder. On an
M4 Max, interleaved runs with rotated ordering, reporting the minimum and first
quartile because the maximum is dominated by unrelated system activity:
max_bin=512 500K x 400 dense 30 rounds 6.141 -> 4.743 (-22.8% min)
max_bin=256 400K x 300 dense 25 rounds 1.362 -> 1.296 ( -4.9% min)
max_bin=256 400K x 400 dense 25 rounds 1.761 -> 1.694 ( -3.8% min)
So a few percent at the default max_bin, growing with max_bin. Shapes that stay
on the row-wise path are unaffected, as expected, since their histograms fit the
L2 budget either way and L2 does not change on this part:
max_bin=256 1M x 50 dense 100 rounds 1.806 -> 1.811 (+0.3% min)
max_bin=256 1M x 100 sparse 100 rounds 2.018 -> 1.999 ( -1.0% min)
max_bin=256 400K x 150 dense 25 rounds 0.607 -> 0.618 (+1.8% min)
Also reformat cache_manager.h, which did not match clang-format.
The CPU histogram builder hardcodes a 64-byte cache line in two places: the prefetch stride in hist_util.cc and the minimum block size in tree/hist/histogram.h. Apple Silicon uses 128-byte lines, so on that target the prefetch loop issues two prefetches per line instead of one, and the minimum block covers half a line, which lets neighbouring blocks processed by different threads share one. Replace both with a single `common::kCacheLineSize` next to the other cache facts, selected per target. std::hardware_destructive_interference_size would be the obvious way to express this, but it is not usable here: libc++ reports 64 for it on arm64 including Apple Silicon, so it would reintroduce exactly the value this constant exists to correct. Its plausibility and power-of-two-ness are checked with static_assert, since both are compile-time properties, and a test asserts the compiled constant against `hw.cachelinesize` so a wrong value fails rather than silently mistuning the build. This is a correctness fix, not a measured speedup. Across 27 interleaved training runs on an M4 Max it made no difference outside noise on any of the three shapes benchmarked, in either direction; the minimum block size rarely binds, because block_size is normally in the hundreds of rows. It is worth having because the constant is simply wrong for the target, and because the false-sharing it allows is a function of thread count and partition layout rather than of these particular shapes. Behaviour on every other target is unchanged; kCacheLineSize is 64 there, as before.
Member
|
Unfortunately, we don't have the capacity to review your optimization PRs at the moment, as none of the active maintainers use Apple silicon. I will look into P/E cores in the future and hopefully gain some understanding of what's happening there. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The CPU histogram builder hardcodes a 64-byte cache line in two places:
src/common/hist_util.cc—Prefetch::kCacheLineSize, which sets the prefetchstride and the size of the no-prefetch tail.
src/tree/hist/histogram.h— a localkCacheLineSizeused to floor the blocksize, with a comment stating its purpose is "to ensure a full cache line is
utilized when loading gradient pairs".
Apple Silicon uses 128-byte lines (
sysctl hw.cachelinesizereports 128). Onthat target both constants work against their own stated intent: the prefetch
loop issues two prefetches per line instead of one, and the minimum block covers
half a line, so neighbouring blocks handed to different threads can share one.
This replaces both with a single
common::kCacheLineSize, placed incache_manager.hnext to the other cache facts and selected per target.Why not
std::hardware_destructive_interference_sizeThat would be the idiomatic way to express this, and it is not usable here.
libc++ reports 64 for it on arm64, including Apple Silicon, so it would
reintroduce the exact value this constant exists to correct. Verified rather
than assumed:
Verification
The two compile-time properties — plausibility and power-of-two-ness — are
checked with
static_assertrather than at runtime, since the compiler canprove both. A separate test asserts the compiled constant against
hw.cachelinesize, so a wrong value fails loudly instead of silently mistuningthe build:
CacheManager.AppleSiliconCacheLineSize— compiled constant vs. what themachine reports.
Performance
This is a correctness fix, not a measured speedup, and I want to be explicit
about that. Across 27 interleaved training runs on an M4 Max — row-wise,
column-wise and sparse shapes, with rotated ordering — it made no difference
outside noise in either direction. The minimum block size rarely binds in
practice, because
block_sizeis normally in the hundreds of rows.It is worth having because the constant is simply wrong for the target, and
because the false sharing it permits is a function of thread count and partition
layout rather than of the three shapes I happened to measure.
Compatibility
No behaviour change on any other target —
kCacheLineSizeis 64 there, exactlyas before. 745 C++ unit tests pass on macOS arm64, and the x86_64 branch was
cross-compiled to confirm it still builds.
Note
Stacks on (
Detect CPU cache sizes on Apple Silicon), which introduces thenew contents of
cache_manager.h. Please merge that one first.