Repository navigation
perf(multi): sweep vector-leaf histograms by feature on sparse indexes - #163
Merged
Merged
Conversation
GHistIndex::column_bins() is Some only for dense indexes, so any matrix with missing values sent every vector-leaf (multi_output_tree) node through the serial CSR loop, whatever the thread count. NodeHistograms now sweeps the feature-major copy whenever the index keeps one: dense columns, or missing_columns() skipping the width's sentinel. Tasks of four pool features read each row's split gradients once, and only the tree's colsample_bytree pool is built (every node's candidate features are a subset of it). Each bin still adds its rows in one ascending chain, so models are bit-identical at every thread count.
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
The feature sweep ran on every node of 65,536+ row x pool entries, on one thread or many, in fixed four-feature tasks. A sweep reads an entry per pool feature (missing ones too) through a column per feature, where a CSR row is contiguous, so it lost to the row build wherever it had no threads to split over: depth-8 trees on 1M rows trained 1.5x (dense) to 1.7x (40% missing) slower than main on one thread, and 1.5x slower on two (40% missing). Fixed four-feature tasks also left a narrow pool's root on a few threads. Each level now gives its nodes shares of the threads by their rows, and a node is swept only when the sweep's entries, weighted 3/2 and split over its share, are fewer than its rows' stored entries; one thread sweeps only pools under two thirds of a row's stored entries. Tasks hold one to four features, as few as keep the node's share busy (`walk::by_features`'s rule). Every bin is still one ascending row chain, so models keep their bits. Pool slices are taken by feature index rather than relying on the pool's order, `partition_numeric` names the missing sentinel through `BinIndex::MISSING`, the sweep test covers 32-bit bins and tasks of every size, and AGENTS.md records that vector-leaf bins are one row chain each and that bins outside the tree's pool are stale.
Owner
Author
|
@codex review |
🛡️ Codex Security Review · Automatically triggeredSecurity review completed. No security issues were found in this pull request. Reviewed commit: Only the user who started this review can view the report in Codex. ℹ️ About Codex security reviews in GitHubThis is an experimental Codex feature. Security reviews are triggered when:
Once complete, Codex will leave suggestions, or a comment if no findings are found. |
|
Codex Review: Didn't find any major issues. Keep it up! Reviewed commit: ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
If Codex has suggestions, it will comment; otherwise it will react with 👍. Codex can also answer questions or update the PR. Try commenting "@codex address that feedback". |
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
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.
Problem
GHistIndex::column_bins()isSomeonly for dense indexes. A matrix with missing values getsColumns::WithMissing, exposed only throughmissing_columns(), somulti::Grow::build_histnever took its feature-parallel branch: every vector-leaf (multi_strategy = multi_output_tree) node ran the serial CSR loop. Per-round time hardly changed with the thread count (861 / 765 / 672 ms at 1 / 4 / 16 threads).Change
Histogram construction moves out of
GrowintoNodeHistograms, which builds a node either row by row over its CSR bins or by sweeping the index's feature-major copy feature by feature: dense columns, ormissing_columns()with the width's sentinel skipped (BinIndex::MISSING, a new associated constant thatpartition_numericnow names too).colsample_bytreepool is swept (ColumnSampler::tree_features). Every node'sbylevel/bynodecandidates are a subset of that pool, so no bin that is read gets skipped. Bins outside the pool are stale and never read.NodeHistograms::sweep_pays): per row, the sweep reads one entry per pool feature, missing ones included, at about 3/2 the cost of a CSR entry, split over the node's share; the CSR loop reads the row's stored entries. So a node with one thread (on a one-thread pool, or on a level whose nodes already occupy every thread) is swept only when its pool is under two thirds of a row's stored entries.walk::by_features's rule), and reads each row's split gradients once for all of them.Each bin still adds its rows in a single ascending-row
f64chain, whichever way a node is built and on however many threads. There are no row blocks, partial histograms or merges, so vector-leaf models are bit-identical tomainat every thread count. Sibling subtraction is unchanged.Review follow-up (second commit)
The first revision (
ddd95a6) swept every node above the entry threshold, on one thread too, in fixed four-feature tasks. On deeper trees that lost to the CSR loop wherever a node had no threads to split over: a sweep of scattered rows reads a column entry per feature where a CSR row is contiguous. Depth-8 trees on 1M rows trained 1.5× (dense) to 1.7× (40% missing) slower thanmainon one thread, and 1.5× slower on two. The fixed task size also left a narrow pool's root on a few threads (multi_output_k3below). The second commit adds the per-node thread shares,sweep_pays, and share-sized tasks; takes pool slices by feature index instead of relying on the pool's order; extends the test to 32-bit bins and every task size; and records the one-chain-per-bin rule and the stale non-pool bins in AGENTS.md.Numbers
Median round time in ms (a throwaway Rust harness over the public API:
Trainer::on_roundtimestamps, median of 10 rounds, 5 for the panel; mean of two interleaved runs). Neoverse-V3, pinned withtaskset. All runs usehist,subsample 0.5,lambda 3,min_child_weight 8, eta 0.12, vector leaves.ddd95a6vecprof.py's), two quantiles, depth 4The head never runs slower than
main(at most 2%, within run-to-run noise). Againstddd95a6it gives back the one-thread gain on shallow trees with a half-feature pool (the panel at one and four threads, the sparse wide case at two), in exchange for the deep-tree regressions above.train_variants_50k_x20_20rounds/multi_output_k3(scripts/compare_benchmarks.py, dense 50k × 20, 3 targets, colsample 1), againstmain:ddd95a6The first revision's end-to-end runs, on
ddd95a6(bands.pyat 16 threads: 2.77M train / 615k valid / 616k test rows, early stopping with patience 40); the head's 16-thread round on this panel equalsddd95a6's (111 ms, above):Bit identity
tree::builder::multi::tests::feature_sweep_matches_the_row_buildcompares every pool bin'sf64bits fromNodeHistograms::buildagainst the CSR row build, on dense and sparse indexes of both bin widths (8 features in 16-bit bins, 300 in 32-bit bins), at 1, 2, 4 and 16 threads (tasks of one to four features). It also checks that all four thread counts grow the same tree. Gradients span 40 binary orders of magnitude, so any reordering changes bits; dropping a feature from three-feature tasks, or a wrong 32-bit sentinel, makes it fail.colsample_bylevel 0.8andcolsample_bynode 0.7, over 8 dense features, and over 12 features 30% missing, gives one native JSON model hash and one margin hash acrossmain,ddd95a6and this head at 1, 2, 4, 16 and 64 threads (the panel at 1, 4, 16 and 64). After mergingmain(feat!: model averaging, out-of-fold cross-validation with per-fold stopping; faster quantile scale pass #165), the head still matches the newmainon the sparse wide, deep sparse and dense wide cases at 1 and 16 threads.cargo nextest run --all-features(878 passed), doctests, clippy and rustdoc with warnings denied, and the 1.93 MSRV build.