[SYSTEMDS-3524] Add multi-threaded unique in LibMatrixSketch - #2542
[SYSTEMDS-3524] Add multi-threaded unique in LibMatrixSketch#2542shieru1214 wants to merge 10 commits into
Conversation
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## main #2542 +/- ##
============================================
+ Coverage 71.41% 71.48% +0.06%
- Complexity 48774 49849 +1075
============================================
Files 1571 1602 +31
Lines 188912 193298 +4386
Branches 37067 37836 +769
============================================
+ Hits 134917 138184 +3267
- Misses 43544 44317 +773
- Partials 10451 10797 +346 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Thank you for the contribution @shieru1214. It looks good overall, but there are some issues I would like you to go over.
First, the most critical problem is that the AggregateUnary instruction will always call the single-threaded implementation. With multi-threading in place, it should be only used as a fallback.
Second, the batched multi-threading path has a number of problems related to memory estimation and guards against excessive usage. Below is the summary of suggested fixes:
- Int overflow: compute as
int end = (int) Math.min((long) pos + config._taskLen, config._len); - The safety condition for
RowandColisnumThreads × cellsPerIndex × 64 ≤ budget(one live row/column set per thread). Otherwise, you force batched execution where it can be safely avoided - You have to keep in mind that for batched
RowColthe mergedHashSetgrows toward the distinct count. So you should sizetaskLenagainst a fraction of the budget with the remainder reserved for the merged set. If the worst-case simply doesn't fit, fall back to sequential execution. - I'm pretty sure batched tests do not test the respective path in our CI since the JVM heap is never sufficiently low (the worst-case requirement is ~77 MiB). You can, e.g., add an overload like
maxLocalBytes, or account for it in any other way you prefer
Also, the formatting job is failing, you need to run the dev/format-changed.sh script. Your fork doesn't have it, so please rebase on the current main so that it's present in your repository as well.
| //similar to R's unique, this operation takes a matrix and computes the | ||
| //unique values (or rows in case of multiple column inputs) | ||
|
|
||
| return getUniqueValues(blkIn, dir, 1); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| for( int pos = 0; pos < config._len; ) { | ||
| ArrayList<UniqueValueTask> tasks = new ArrayList<>(); | ||
| for( int i = 0; i < config._numThreads && pos < config._len; i++ ) { | ||
| int end = Math.min(pos + config._taskLen, config._len); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| for( int pos = 0; pos < config._len; ) { | ||
| ArrayList<UniqueCountTask> tasks = new ArrayList<>(); | ||
| for( int i = 0; i < config._numThreads && pos < config._len; i++ ) { | ||
| int end = Math.min(pos + config._taskLen, config._len); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| for( int pos = 0; pos < config._len; ) { | ||
| ArrayList<UniqueOutputTask> tasks = new ArrayList<>(); | ||
| for( int i = 0; i < config._numThreads && pos < config._len; i++ ) { | ||
| int end = Math.min(pos + config._taskLen, config._len); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
|
|
||
| ExecutorService pool = CommonThreadPool.get(config._numThreads); | ||
| try { | ||
| HashSet<Double> hashSet = new HashSet<>(); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| if( numThreads <= 1 ) | ||
| return null; | ||
|
|
||
| int taskLen = Math.max(1, (int) Math.min(Integer.MAX_VALUE, maxBatchIndexes / numThreads)); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * @return true if local deduplication is small enough for the parallel path | ||
| */ | ||
| private static boolean isLocalDedupMemoryBudgetSafe(MatrixBlock blkIn, Types.Direction dir) { | ||
| return getMaxLocalDedupIndexes(blkIn, dir) >= getPartitionLength(blkIn, dir); |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| * under the License. | ||
| */ | ||
|
|
||
| package org.apache.sysds.runtime.matrix.data; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
| return 0; | ||
|
|
||
| long bytesPerIndex = cellsPerIndex * Double.BYTES * PAR_UNIQUE_LOCAL_BYTES_OVERHEAD; | ||
| long maxLocalBytes = Runtime.getRuntime().maxMemory() / PAR_UNIQUE_MAX_LOCAL_BYTES_FRACTION; |
This comment was marked as resolved.
This comment was marked as resolved.
Sorry, something went wrong.
…hed memory guards - AggregateUnaryCPInstruction now passes op.getNumThreads() into LibMatrixSketch.getUniqueValues, so the multi-threaded implementation is actually used instead of always falling back to the single-threaded path. - Fix int overflow in the batched task-range computation ((int) Math.min((long) pos + taskLen, len)). - Correct the memory guard for the row-wise and column-wise paths: their workers reuse one set that is cleared per row/column, so only one live set per thread is charged (numThreads * cellsPerIndex * 64 <= budget) instead of charging all indexes, which forced batched execution where it is safely avoidable. - For batched RowCol reserve the all-distinct worst case of the merged set up front and size taskLen against the remaining budget; fall back to sequential execution when even the merged set does not fit. - Add a getUniqueValues overload with an injectable maxLocalBytes budget, move the parallel test into functions.unique so CI actually runs it, and add cases that deterministically exercise all three batched paths and their sequential fallbacks (the heap-derived default budget never triggers them in CI).
Derive the transient deduplication budget from InfrastructureAnalyzer.getLocalMaxMemory() instead of Runtime.getRuntime().maxMemory(). The analyzer reflects the local JVM memory that parfor adjusts per worker, so the raw JVM maximum would over-estimate the budget available to a parfor worker. This also matches the convention used by the neighbouring matrix libraries (e.g. LibMatrixMult). Also expand the direction dispatch into explicit if/return branches so the formatter does not wrap the calls mid-argument.
Adds a benchmark in the existing performance package that compares unique() against the k=1 baseline across different shares of unique values, for all three directions (RowCol, Row, Col) and a small and a large input, at a fixed degree of parallelism. The share of unique values is interpreted per direction, matching what that direction deduplicates: distinct values over the whole input for RowCol, per row for Row and per column for Col. Each configuration is warmed up and then measured repeatedly, the median is reported, the order of the baseline and parallel runs is alternated to reduce ordering effects, and input generation is excluded from the measurements.
Update: review feedback addressedThanks for the detailed review. All points are addressed: 1. 2. Int overflow in the batched task ranges. 3. Safety condition for 4. Batched 5. The batched paths were not covered in CI. Additionally, the memory budget is now derived from The branch is rebased on current Testing
19 tests pass in total. Checkstyle reports 0 violations and the format check on the PR-edited lines is clean. Runtime experimentsThe benchmark is included as The experiments were run locally on an Intel Core i7-13700H (20 logical cores) using Java 17, with the JVM heap fixed at 3 GB and dense For each configuration I used three warm-up iterations followed by five measured iterations. The order of the The experiments cover the directions These are local measurements and are intended as an initial comparison rather than a full benchmark. RowCol
Row
Col
|
gaturchenko
left a comment
There was a problem hiding this comment.
Thank you for the fixes @shieru1214 @Isso-W, we appreciate your effort. I added a comment regarding the test file, please address when you can.
There was a problem hiding this comment.
This test file is rather inconsistent with the general structure of function tests. The biggest issue is that you don't actually invoke a DML script but rather call the instruction directly. So essentially instead of end-to-end you have unit tests, and we generally prefer the former. Therefore, please have another look at how existing unique tests are implemented. If necessary, you can extend the UniqueBase class. Regarding the tests that constrain the memory budget to trigger the batched path, you can in principle leave them as they are, but I'd still appreciate if you could find a way to make them DML-based as well.
Replaces the unit-style parallel test with end-to-end tests that follow the structure of the existing unique function tests, i.e. they invoke the DML script instead of calling the instruction directly. - adds uniqueCol.dml and UniqueCol, so the column-wise direction has script level coverage at all - extends UniqueBase with an ordered comparison, for cases where every row or column of the result holds a single value and the hash set iteration order cannot affect the outcome, and with a variant that temporarily reduces the local memory budget - adds multi-threaded cases for all three directions, and a batched RowCol case that reaches the memory-aware path from a script run - keeps a small unit test for the batched row-wise and column-wise paths only. Those select batching when numThreads * cellsPerIndex * 64 exceeds the budget, which from an end-to-end run would require the budget to fall into a narrow window depending on the thread count of the executing machine.
The multi-threaded cases so far only covered dense inputs. This adds a sparse counterpart for each direction, following the dense/sparse split of the countDistinct tests. Only every eighth row, every eighth column or every tenth cell is populated, so the input is read in sparse format while every row or column still holds a single distinct value and the expected result stays unambiguous.
|
Update: tests are now script-based Thanks, that was a fair point. The parallel test was indeed a unit test in disguise. It is removed and replaced with end-to-end tests that follow the existing unique tests. What changed
24 cases in total, 22 of them run the DML script. Batched paths
For Happy to drop that test if you would rather have the two paths uncovered. Verification I instrumented the path selection temporarily to confirm the tests reach what they claim: the script runs use |
There was a problem hiding this comment.
Hey @shieru1214 @Isso-W, some more comments from my side. NB: since we are already past the project submission deadline, please feel free to take as much time as you need to make the changes. Also, if some of my comments are unclear or you disagree with them, feel absolutely free to respond, we can certainly have a discussion about this.
|
|
||
| boolean localDedupMemorySafe = isLocalDedupMemoryBudgetSafe(blkIn, dir, k, maxLocalBytes); | ||
| switch(dir) { | ||
| case RowCol: |
There was a problem hiding this comment.
Sorry, I missed this one in my prior review. Based on your benchmarking results, it seems that multi-threading yields a slowdown for RowCol. Can we then just fall back to single-threaded here?
| * unambiguous, i.e. where every row or column of the result holds a single value and the iteration order of the | ||
| * internal hash sets cannot affect the outcome. | ||
| */ | ||
| protected void uniqueTestOrdered(double[][] inputMatrix, double[][] expectedMatrix, Types.ExecType instType, |
There was a problem hiding this comment.
Why do you need this separate function? The existing uniqueTest function compares the results out-of-order that works both when the output is deterministic or random. Am I missing something here?
| writeExpectedMatrix("A", expectedMatrix); | ||
|
|
||
| compareResultsRowsOutOfOrder(epsilon); | ||
| if(orderedComparison) |
There was a problem hiding this comment.
Again as per my comment above, I don't really understand the purpose of orderedComparison
There was a problem hiding this comment.
Why is this still a unit test? You have limited the memory with InfrastructureAnalyzer but you can limit concurrency as well by calling InfrastructureAnalyzer.setLocalPar(), so I don't quite see what holds you back from exercising the batched path for all 3 cases end-to-end
| * is one row and independent of any hash set iteration order. | ||
| */ | ||
| @Test | ||
| public void testMultiThreadedCP() { |
There was a problem hiding this comment.
Thank you for covering the Col case as well. However, what do you mean by large enough? The multi-threaded execution should be picked up by default unless the constraints are violated. Why are the 4 existing tests insufficient?
| * against needlessly falling back to batched or sequential execution. | ||
| */ | ||
| @Test | ||
| public void testReducedMemoryBudgetCP() { |
There was a problem hiding this comment.
This should rather be in the batched test, although in principle you compare the results here, but do not actually check that the batched path was selected. So maybe if there is no way to verify it we don't need this test at all
| * one column and independent of any hash set iteration order. | ||
| */ | ||
| @Test | ||
| public void testMultiThreadedCP() { |
There was a problem hiding this comment.
Same comment as for Col
| * needlessly falling back to batched or sequential execution. | ||
| */ | ||
| @Test | ||
| public void testReducedMemoryBudgetCP() { |
There was a problem hiding this comment.
Same comment as for Col
| * one column. | ||
| */ | ||
| @Test | ||
| public void testSparseMultiThreadedCP() { |
There was a problem hiding this comment.
What do you verify with this test? Why is this case different from the other tests?
There was a problem hiding this comment.
If multi-threading is not beneficial for RowCol, we don't need any new tests here
Summary
This PR adds a multi-threaded path for
uniqueinLibMatrixSketch.The existing single-threaded implementation is kept and is still used as the baseline/fallback. I added a new overload:
getUniqueValues(MatrixBlock blkIn, Types.Direction dir, int k), wherekcontrols the requested number of threads.The implementation follows the existing SystemDS
uniquedirection semantics:RowCol: unique scalar values over the input matrix, returned as one columnRow: row-wise unique scalar values, preserving the number of rowsCol: column-wise unique scalar values, preserving the number of columnsImplementation
For
k > 1and sufficiently large inputs, the code uses balanced row or column ranges andCommonThreadPool.The parallel logic is direction-specific:
RowCol: each worker builds a localHashSet<Double>over its row range, and the local sets are merged afterwards.Row: rows are partitioned across workers. The code first computes the maximum number of unique values per row, allocates the output, and then fills the row-wise unique values in parallel.Col: columns are partitioned across workers. The code first computes the maximum number of unique values per column, allocates the output, and then fills the column-wise unique values in parallel.There are two parallel paths:
The batched path processes a limited number of ranges at a time, clears temporary local data earlier, and then continues with the next batch. If the input is small,
k <= 1, or batching is not useful, the code falls back to the original single-threaded path.Testing
I added a focused JUnit test:
src/test/java/org/apache/sysds/runtime/matrix/data/LibMatrixSketchUniqueParallelTest.javaIt checks
RowCol,Row, andCol, and compares the multi-threaded result with the single-threaded baseline.I ran the following tests:
mvn -Dtest=LibMatrixSketchUniqueParallelTest testmvn -Dtest=UniqueRow,UniqueRowCol testThe focused test completed with 4 tests passing, and the existing unique tests completed with 9 tests passing. I also ran the focused test with
-Xmx64mto exercise the memory-aware batched path.Runtime experiments
I ran local runtime experiments with fixed
k=4, comparing against thek=1baseline.The experiments were run locally on an Apple Silicon Mac using Java 17. The JVM heap was fixed at 3 GB, and all inputs used dense
MatrixBlockstorage.For each case, I used three warm-up iterations followed by five measured iterations. The order of
k=1andk=4was alternated between iterations to reduce ordering effects, and the median runtime is reported. Input generation is not included in the measurements.The experiments cover:
RowCol,Row, andCol1%,10%,50%, and100%k=1andk=4In the tables,
Unique Valueshas a different meaning for each direction:RowCol, it is the total number of distinct scalar values in the inputRow, it is the number of distinct scalar values within each rowCol, it is the number of distinct scalar values within each columnThe exact integer count used for each case is shown in the tables. Speedup is calculated as
k=1 time / k=4 time, so a value above 1 means that the multi-threaded path was faster.The 3 GB heap limit was chosen to keep the large dense inputs within memory while allowing the memory guard to select the batched path for the large
RowandColcases. With this setting, the smaller cases use the regular parallel path, while the largeRowandColcases use batched parallel processing.These are local measurements and are intended as an initial comparison rather than a full benchmark.
RowCol
Row
Col
The row-wise and column-wise cases show consistent speedups because rows and columns can be processed independently. Their speedup generally decreases as the number of unique values grows, since larger sets and outputs require more work.
RowColbehaves differently because its thread-local sets must still be merged into one global set. The large input with a 1% unique share shows a small improvement, but in the other tested cases the allocation and merge overhead is greater than the saved parallel work.