refactor: replace merge-sort with heapsort #405
Merged
Conversation
This file contains 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
This change replaces the merge-sort with a heapsort that uses much less CU than merge-sort. The previous algorithm is very fast running on a normal CPU but doesn't work very well in BPF because all instructions (like load, move, ..) have the same cost and there is no cache and optimizations for memory-alignment have no real benefit. A major benefit of heapsort is being non-recursive that reduces the high stackframe overhead in BPF and is inplace which minimizes number of copies. Unfortunately there is no way to systematically get the the compute usage out of program test. The `test_benchmark` file has a simple code that helps running benchmarks on various number of publishers. In 32-publisher setup, heapsort reduces the CU from 16.5k to 12k and in the 64-publisher setup 37k to 20.5k. The numbers are the worst cases running on randomized input.
ali-bahjati
force-pushed
the
optimize/use-heapsort
branch
2 times, most recently
from
f04cafb
to
0302196
Compare
ali-bahjati
force-pushed
the
optimize/use-heapsort
branch
from
0302196
to
c581da7
Compare
guibescos
reviewed
May 15, 2024
| @@ -188,8 +188,7 @@ static inline bool upd_aggregate( pc_price_t *ptr, uint64_t slot, int64_t timest | |||
| // note: numv>0 and nprcs = 3*numv at this point | |||
| int64_t agg_p25; | |||
| int64_t agg_p75; | |||
| int64_t scratch[ PC_NUM_COMP * 3 ]; // ~0.75KiB for current PC_NUM_COMP (FIXME: DOUBLE CHECK THIS FITS INTO STACK FRAME LIMIT) | |||
There was a problem hiding this comment.
Great that it's in place now
guibescos
reviewed
May 15, 2024
ali-bahjati
force-pushed
the
optimize/use-heapsort
branch
from
3e13613
to
9afe6e8
Compare
ali-bahjati
force-pushed
the
optimize/use-heapsort
branch
from
9afe6e8
to
5647ad0
Compare
Reisen
previously approved these changes
May 16, 2024
ali-bahjati
added a commit
that referenced
this pull request
May 16, 2024
This reverts commit 2ea67f3.
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.
This change replaces the merge-sort with a heapsort that uses much less CU than merge-sort.
The previous algorithm is very fast running on a normal CPU but doesn't work very well in BPF because all instructions (like load, move, ..) have the same cost and there is no cache and optimizations for memory-alignment have no real benefit.
The major benefits of heapsort are being non-recursive that reduces the high stackframe overhead in BPF and being inplace which minimizes number of copies.
Unfortunately there is no way to systematically get the the compute usage out of program test. The
test_benchmarkfile has a simple code that helps running benchmarks on various number of publishers.In 32-publisher setup, heapsort reduces the CU from 16.5k to 12k and in the 64-publisher setup 37k to 20.5k. The numbers are the worst cases running on randomized input. The result of running in a highly similar input (two distinct prices, and two distinct confidences) is 14.7k and 25k respectively, which is still better and is very unlikely in practice.