[Java] VariableWidthViewVectorBenchmarks

[ViggoC]

The performance of VariableWidthViewVector is much slower than VariableWidthVector in some cases. setZero for overwriting is the bottleneck.

VariableWidthViewVectorBenchmarks.setSafeFromArray       1  avgt    5  0.088 ± 0.002  ms/op
VariableWidthViewVectorBenchmarks.setSafeFromArray       2  avgt    5  0.095 ± 0.020  ms/op
VariableWidthViewVectorBenchmarks.setSafeFromArray      10  avgt    5  0.091 ± 0.012  ms/op
VariableWidthViewVectorBenchmarks.setSafeFromArray      40  avgt    5  0.087 ± 0.001  ms/op
    VariableWidthVectorBenchmarks.setSafeFromArray       1  avgt    5  0.010 ±  0.001  ms/op
    VariableWidthVectorBenchmarks.setSafeFromArray       2  avgt    5  0.013 ±  0.001  ms/op
    VariableWidthVectorBenchmarks.setSafeFromArray      10  avgt    5  0.032 ±  0.001  ms/op
    VariableWidthVectorBenchmarks.setSafeFromArray      40  avgt    5  0.105 ±  0.002  ms/op

Environment: 2.6 GHz Intel Core i7, MacOS 13.6, openjdk version "13.0.2"
See code of benchmark in apache/arrow#44634.

Component(s)

Java

[lidavidm]

Thanks for digging into this @ViggoC! I think we can take out the setZero since unreferenced data is unspecified

[ViggoC]

@lidavidm So we can just remove it without side effort?

[lidavidm]

What do you mean side effort?

[vibhatha]

Is it side effect?

[lidavidm]

Oh, good point.

Technically there would be a side effect in that data could be left over in the buffer, and then make it into IPC, but that is the tradeoff. (I believe it's already possible if say you create a string vector, add data, and later set the row count to something lower than before - the extra data won't be cleared.)

[vibhatha]

Technically there would be a side effect in that data could be left over in the buffer, and then make it into IPC, but that is the tradeoff. (I believe it's already possible if say you create a string vector, add data, and later set the row count to something lower than before - the extra data won't be cleared.)

Yeah that make sense.

And @ViggoC thanks for looking into this, I am looking into the PR as well. This is much needed to improve the initial version.

[ViggoC]

@lidavidm sorry, my bad, I mean side effect.
@vibhatha so you'll handle this performance issue?

[ViggoC]

@vibhatha And I also see that VariableWidthViewVector allocated much more memory than VariableWidthVector in some cases. It runs out of memory(8G) before I changed the allocator.close to iteration level.

[lidavidm]

Hmm, I think we still allocate power-of-two buffers for each of the data buffers, probably we should instead round to some multiple of say 64K or 1 MB or something instead. The implementation is still new so we are working out how best to treat it.

[vibhatha]

@ViggoC technically the answer is yes, I can do it, but in a practical sense my phase would be significantly slower than earlier because the time I could allocate would be significantly smaller. If this needs fixing sooner, the best I can do is help to figure out things to do and definitely review the PR if you could work on this feature.

[ViggoC]

IMHO, The only side effect of removing viewBuffer.setZero is that padding data is no longer 0, Right?

  /*
   * Variable Width View Vector comprises the following format
   *
   * Short strings, length <= 12
   * | Bytes 0-3  | Bytes 4-15                            |
   * |------------|---------------------------------------|
   * | length     | data (padded with 0)                  |
   * |------------|---------------------------------------|
   *
   * Long strings, length > 12
   * | Bytes 0-3  | Bytes 4-7  | Bytes 8-11 | Bytes 12-15 |
   * |------------|------------|------------|-------------|
   * | length     | prefix     | buf.index  | offset      |
   * |------------|------------|------------|-------------|
   *
   * */

[lidavidm]

ah...are the requirements for variable width vector different? if they must be 0-padded then we can't do anything here.

[lidavidm]

No, that diagram only applies to short strings. It's not specified what happens to long strings.

[vibhatha]

It would be better to write some test cases and evaluate this.

[ViggoC]

if they must be 0-padded then we can't do anything here.

"Any remaining bytes after the string itself are padded with 0."
https://arrow.apache.org/docs/format/Columnar.html#:~:text=Any%20remaining%20bytes%20after%20the%20string%20itself%20are%20padded%20with%200.

[ViggoC]

No, that diagram only applies to short strings. It's not specified what happens to long strings.

yeah, when i mentioned padding, i mean short string, there is no padding data for long string. for long string, every bytes in viewbuffer will be overwritten. so at least we can skip viewbuffer.setZero for long string.

[ViggoC]

commit 30e595411e37b7ca2739c7005442ed7b329eeaea. the benchmark only involves long string, so it shows a big improvement.

Benchmark                                           (step)  Mode  Cnt  Score   Error  Units
VariableWidthViewVectorBenchmarks.setSafeFromArray       1  avgt    5  0.024 ± 0.002  ms/op
VariableWidthViewVectorBenchmarks.setSafeFromArray       2  avgt    5  0.024 ± 0.002  ms/op
VariableWidthViewVectorBenchmarks.setSafeFromArray      10  avgt    5  0.024 ± 0.003  ms/op
VariableWidthViewVectorBenchmarks.setSafeFromArray      40  avgt    5  0.024 ± 0.008  ms/op