Improve performance of VariableWidthBlock#copyPositions#10409
Improve performance of VariableWidthBlock#copyPositions#10409sopel39 merged 2 commits intotrinodb:masterfrom
Conversation
skrzypo987
left a comment
There was a problem hiding this comment.
I am afraid there will be a regression for certain position arrays. Is there any benchmark?
IMHO it would be best to apply some heuristics to guess if the "new" way of copying will be faster e.g. some function of fill ratio and average position length. We can also fallback to old code if the first loop uncovers highly fragmented positions array.
8654fb2 to
f7acdc9
Compare
I've tweaked the code a bit and posted JMH results with different selection patterns. It's doing better in all the benchmarked scenarios. |
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/VariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/VariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/VariableWidthBlock.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
I don't think offsets are guaranteed to make sense for null positions (add a test?)
There was a problem hiding this comment.
The bytes at a particular offset in slice for a null position can be junk, we don't care about that. However, the offset positions need to be valid in existing code as well, otherwise the result of getSliceLength for the next non-null position could go wrong.
f7acdc9 to
f7426eb
Compare
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/BenchmarkVariableWidthBlock.java
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Not sure, I just copied this one from BenchmarkDataGenerator#randomNullChance
@skrzypo987 do you know why ?
There was a problem hiding this comment.
There is a < at the end, so if random picks 0 it will return null for null chance of 0.
We basically want to pick random out of [0,1>, but nextBoolean returns <0,1>
core/trino-spi/src/main/java/io/trino/spi/block/VariableWidthBlock.java
Outdated
Show resolved
Hide resolved
Instead of copying every position one at a time, copy contiguous ranges of Slice to reduce the number of times SliceOutput#writeBytes is called when there are sequences in the positions being copied. Reduce branches for populating null values BenchmarkVariableWidthBlock.copyPositions (nullsAllowed) (selectedPositions) (selectedPositionsCount) us/op Before us/op After FALSE GROUPED 200 7.108 4.222 FALSE GROUPED 1000 48.71 22.427 FALSE GROUPED 8000 476.058 186.539 FALSE SEQUENCE 200 6.912 4.107 FALSE SEQUENCE 1000 47.931 21.282 FALSE SEQUENCE 8000 473.199 174.198 FALSE RANDOM 200 8.175 7.505 FALSE RANDOM 1000 53.125 51.679 FALSE RANDOM 8000 519.651 478.773 TRUE GROUPED 200 5.897 4.099 TRUE GROUPED 1000 38.14 19.807 TRUE GROUPED 8000 394.617 158.567 TRUE SEQUENCE 200 6.06 3.508 TRUE SEQUENCE 1000 39.77 17.536 TRUE SEQUENCE 8000 413.274 149.79 TRUE RANDOM 200 7.076 7.201 TRUE RANDOM 1000 42.756 43.626 TRUE RANDOM 8000 416.765 385.222
f7426eb to
769f74c
Compare
Instead of copying every position one at a time, copy
contiguous ranges of Slice to reduce the number of times
SliceOutput#writeBytes is called when there are sequences
in the positions being copied.
Reduce branches for populating null values
BenchmarkVariableWidthBlock.copyPositions