Improve partitioned output operator performance for strings

[lukasz-stec]

Description

Improve performance of SlicePositionsAppender.
The improvement comes from not allocating Slice per position + using System.arrayCopy directly instead of Slice.getBytes

Is this change a fix, improvement, new feature, refactoring, or other?

performance improvement

Is this a change to the core query engine, a connector, client library, or the SPI interfaces? (be specific)

core query engine (partitioned exchange) and SPI extension

How would you describe this change to a non-technical end user or system administrator?

Improve performance of remote partitioned exchange

Benchamrks

jmh

about 30% improvement

Before
Benchmark                                   (channelCount)  (enableCompression)  (nullRate)  (partitionCount)  (positionCount)   (type)  Mode  Cnt     Score     Error  Units
BenchmarkPartitionedOutputOperator.addPage               1                false           0                16             8192  VARCHAR  avgt   20  2137.367 ± 166.834  ms/op
BenchmarkPartitionedOutputOperator.addPage               1                false         0.2                16             8192  VARCHAR  avgt   20  1660.550 ±  24.274  ms/op

After
BenchmarkPartitionedOutputOperator.addPage               1                false           0                16             8192  VARCHAR  avgt   20  1399.476 ± 181.301  ms/op
BenchmarkPartitionedOutputOperator.addPage               1                false         0.2                16             8192  VARCHAR  avgt   20  1194.077 ± 160.496  ms/op

tpch/tpcds

Overall there is a small improvement. This is expected as PartitionedOutputOperator is only around 5% of total CPU time and this pr only improves varchars.
When looking at the PartitionedOutputOperator stats only, the improvement is ~10%.

Before
Total: 980917.6656576941
PartitionedOutputOperator: 43522.99081258998, 0.044369667645253706%
After
Total: 974919.225831081
PartitionedOutputOperator: 38515.177167889924, 0.03950601870124904%

poo-slice-orc-part-sf1K.pdf

Documentation

(x) No documentation is needed.
( ) Sufficient documentation is included in this PR.
( ) Documentation PR is available with #prnumber.
( ) Documentation issue #issuenumber is filed, and can be handled later.

Release notes

( ) No release notes entries required.
(x) Release notes entries required with the following suggested text:

# Section
* Improve performance of remote partitioned exchange on string data types. ({issue}`12798`)

[lukasz-stec]

comments answered.

[lukasz-stec]

comments addressed

[lukasz-stec]

Made it non-draft as @raunaqmorarka is reviewing it anyway.

[lukasz-stec]

I rebased on the master (there were some changes to the SlicePositionsAppender) and as discussed offline I reverted the BatchByteWriter change in favor of exposing raw Slice and offsets from the VariableWidthBlock.

[lukasz-stec]

I also updated the jmh results as the absolute number changed significantly after rebasing on the master and running the benchmarks on java 11.0.15 instead of 11.0.11. The relative improvements are similar (slightly better).

[raunaqmorarka]

minor comments, looks good overall

[raunaqmorarka]

nit: Could be simplified to something like

int currentStartOffset = startOffset + length;
for (int i = 0; i < count; i++) {
    offsets[positionCount + i + 1] = currentStartOffset;
    currentStartOffset += length;
}

[lukasz-stec]

I missed this comment.
applied now.

[raunaqmorarka]

An alternative way here is to copy only once into the byte array from Slice using this method, then use System.arrayCopy to copy bytes within the byte array with doubling size at each step, so you get only log_2(count) calls to System.arrayCopy overall.
Not sure if it'll be faster though.

[lukasz-stec]

This is a good idea.
First of all, it allows us to not rely on Slice being backed by a byte array.
Second, I did a quick benchmark and this method is ridiculously fast on small to medium chunk sizes.
I suspect it's not gonna be that used in practice but I included this in this PR given results below.

Benchmark                               (length)  Mode  Cnt      Score   Error  Units
BenchmarkDuplicateBytes.arrayCopy              2  avgt   20      4.101 ±   0.082 ns/op
BenchmarkDuplicateBytes.arrayCopy             16  avgt   20      5.088 ±   0.055 ns/op
BenchmarkDuplicateBytes.arrayCopy             35  avgt   20      4.389 ±   0.175 ns/op
BenchmarkDuplicateBytes.arrayCopy             32  avgt   20      4.333 ±   0.049 ns/op
BenchmarkDuplicateBytes.arrayCopy             64  avgt   20      4.900 ±   0.050 ns/op
BenchmarkDuplicateBytes.arrayCopy             80  avgt   20      5.609 ±   0.117 ns/op
BenchmarkDuplicateBytes.arrayCopy            128  avgt   20      5.318 ±   0.069 ns/op
BenchmarkDuplicateBytes.arrayCopy            200  avgt   20      6.087 ±   0.098 ns/op
BenchmarkDuplicateBytes.arrayCopy            256  avgt   20      6.971 ±   0.111 ns/op
BenchmarkDuplicateBytes.arrayCopy            512  avgt   20     12.175 ±   0.324 ns/op
BenchmarkDuplicateBytes.arrayCopy           1024  avgt   20     22.759 ±   0.300 ns/op
BenchmarkDuplicateBytes.arrayCopy           2048  avgt   20     55.769 ±   1.213 ns/op
BenchmarkDuplicateBytes.arrayCopy         100000  avgt   20   4931.827 ±  276.57 ns/op
BenchmarkDuplicateBytes.arrayCopy        1000000  avgt   20  70111.621 ± 922.907 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x         2  avgt   20      0.230 ±   0.005 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x        16  avgt   20      0.352 ±   0.015 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x        35  avgt   20      0.533 ±   0.052 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x        32  avgt   20      0.498 ±   0.007 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x        64  avgt   20      0.881 ±   0.013 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x        80  avgt   20      1.100 ±   0.015 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x       128  avgt   20      1.518 ±   0.228 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x       200  avgt   20      3.522 ±   0.069 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x       256  avgt   20      4.472 ±   0.154 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x       512  avgt   20     10.083 ±   0.206 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x      1024  avgt   20     21.741 ±   0.349 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x      2048  avgt   20     57.329 ±   0.854 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x    100000  avgt   20   5243.664 ± 188.963 ns/op
BenchmarkDuplicateBytes.copyFromSelf2x   1000000  avgt   20 109400.739 ± 992.325 ns/op

[raunaqmorarka]

CI hit #13107

[skrzypo987]

Looks ok % comment about making getRawSlice method public

[sopel39]

Isn't this already covered by adaptation?

[lukasz-stec]

what adaptation?

[sopel39]

type tests with block adaptation

[lukasz-stec]

Ah, this new one. So no, it's not covered by normal tests, even with adaptation. Like I mentioned here I need to access SlicePositionsAppender directly because UnnestingPositionsAppender copies RLE value to byte array always.
That said, in the current implementation I don't rely on the byte array in the RLE block anymore so I could drop this test

[lukasz-stec]

comments addressed

[lukasz-stec]

what adaptation?

[sopel39]

rename test name, if it's not byte array, then what it is?

Why the test is important (please add short comment)

[lukasz-stec]

rename test name, if it's not byte array, then what it is?

the core of this test is that it is not a byte array and not that it is e.g. long array.
comment added.

[sopel39]

rename, NotByteArray -> WithSomeType

[lukasz-stec]

I dropped this test. it's no longer needed.

[sopel39]

everything should support supportsNullRle

[lukasz-stec]

ArrayBlockBuilder does not support it

[sopel39]

ArrayBlockBuilder does not support it

Does it make sense to add support there? It seems odd that we have to work around that particular type here since all other types (including complex ones) support it.

[lukasz-stec]

ok, created #13973

[lukasz-stec]

moved changes from #13973 to the first commit here + adjusted the tests

[lukasz-stec]

ca

[lukasz-stec]

rename test name, if it's not byte array, then what it is?

the core of this test is that it is not a byte array and not that it is e.g. long array.
comment added.

[lukasz-stec]

I dropped this test. it's no longer needed.

[lukasz-stec]

ArrayBlockBuilder does not support it

[sopel39]

lgtm % comments

[lukasz-stec]

ca

[sopel39]

Added comment

[sopel39]

% comment

[sopel39]

testAppendSliceNotSupportedByByteArray -> testAppendSliceNotBackedByByteArray

[lukasz-stec]

done

[sopel39]

conflict, please rebase

[lukasz-stec]

conflict, please rebase

rebased, checks in progress