Make block_size of BuildHistKernel adaptive #11808
Conversation
|
Thank you for the optimizations! The code looks reasonable, but please add comments when the PR is ready for review. (and ping me). |
src/tree/hist/histogram.h
Outdated
| std::size_t occupied_space = (hist_fit_to_l1 ? hist_size : 0) + offsets_size + idx_bin_size; | ||
| space_in_l1_for_rows = usable_l1_size > occupied_space ? usable_l1_size - occupied_space : 0; | ||
| } | ||
| std::size_t block_size = std::max<std::size_t>(1, space_in_l1_for_rows / l1_row_foot_print); |
There was a problem hiding this comment.
Previously block_size was always 256 rows, which is quite large. And now it is 1 row in case no more rows fit into L1. Won't this change affect the performance in the case when there are no enough space for rows in L1?
Should it be max(256, space_in_l1_for_rows / l1_row_foot_print) ?
Or maybe L2 size should be used to calculate the block_size?
There was a problem hiding this comment.
I think, cacheline_size / (2 * sizeof(float) = 8, would be the best value in this case. Using L2 would result to a huge block_size (~1e4-1e5) and produce potential underutilization of CPU cores (blocks are processed in parallel, and if blocks a very big, than some cores would be out of job).
Co-authored-by: Victoriya Fedotova <[email protected]>
Co-authored-by: Victoriya Fedotova <[email protected]>
Co-authored-by: Victoriya Fedotova <[email protected]>
|
Hi @trivialfis , this PR is ready for review. |
done |
| GetCacheInfo(cache_num++, &type, &level, &sets, &line_size, &partitions, &ways); | ||
| if (!trust_cpuid) return trust_cpuid; | ||
|
|
||
| if (type == kCpuidTypeNull) break; |
There was a problem hiding this comment.
Is the cache_sizes[idx] valid if we break here?
There was a problem hiding this comment.
In this case we use default values from SetDefaultCaches
| size_t hist_size = 2 * sizeof(double) * nbins; | ||
| const bool hist_fit_to_l2 = 0.8 * cache_manager_.L2Size() > hist_size; | ||
|
|
||
| bool read_by_column = !hist_fit_to_l2 && gidx.IsDense(); |
There was a problem hiding this comment.
Could you please add a comment on how this decision is made?
| std::size_t n_bins = gidx.cut.Ptrs().back(); | ||
| std::size_t n_columns = gidx.cut.Ptrs().size() - 1; | ||
| bool any_missing = !gidx.IsDense(); | ||
| std::size_t hist_size = 2 * sizeof(double) * n_bins; |
There was a problem hiding this comment.
Consider using sizeof(GradientPair) and sizeof(GradientPairPrecise) instead of sizeof(float) * 2 (for all sizeof calls in this PR).
src/tree/hist/histogram.h
Outdated
| */ | ||
|
|
||
| /* First step: determine whether one histogram column fits into L1. | ||
| * The maximum number of elements in a column is 2^8, 2^16, or 2^32, |
There was a problem hiding this comment.
Could you please elaborate on what it means to be the maximum number of elements in a (histogram) column? I thought that's the number of histogram bins?
There was a problem hiding this comment.
you are right, bins is a correct term. I have fixed the description.
There was a problem hiding this comment.
Thank you for updating the comments. It's still not clear to me what it means to have "maximum number of bins" in a column. So, what happens if I specify the training parameter max_bin=53?
3 participants
Current version of xgboost utilizes fixed block_size = 256 for hist building.
This PR make this value an adaptive function of model parameters and CPU cache size. The change is important mostly for ColsWiseBuildHistKernel and demonstrates up to 2x speed-up for epsilon dataset.