Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Dont use row block field block offsets if not needed #10825

Merged
merged 2 commits into from
Feb 23, 2022
Merged

Dont use row block field block offsets if not needed #10825

merged 2 commits into from
Feb 23, 2022

Conversation

radek-kondziolka
Copy link
Contributor

@radek-kondziolka radek-kondziolka commented Jan 27, 2022 •
edited
Loading

  • Add support for RowType to BenchmarkDataGenerator and benchmarks
  • Do not use/serialize/deserialize fieldBlockOffset when not needed
    • When there are no null rows within RowBlock then it's not needed to store fieldBlockOffsets.
      This improves performance and reduces network traffic as fieldBlockOffsets array is not serialized or deserialized.
Benchmark  (before/after)                      (nullChance)  Mode  Cnt  Score   Error  Units
(before)   BenchmarkBlockSerde.deserializeRow  0             avgt  60   17.348  ±      0.109  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  0             avgt  60   15.205  ±      0.084  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .01           avgt  60   18.020  ±      0.104  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .01           avgt  60   18.279  ±      0.113  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .10           avgt  60   16.859  ±      0.140  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .10           avgt  60   16.494  ±      0.157  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .50           avgt  60   10.273  ±      0.044  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .50           avgt  60   10.491  ±      0.063  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .90           avgt  60   4.351   ±      0.020  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .90           avgt  60   4.311   ±      0.013  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .99           avgt  60   2.919   ±      0.027  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .99           avgt  60   2.851   ±      0.008  ns/op
(before)   BenchmarkBlockSerde.serializeRow    0             avgt  60   20.862  ±      0.117  ns/op
(after)    BenchmarkBlockSerde.serializeRow    0             avgt  60   16.941  ±      0.089  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .01           avgt  60   21.588  ±      0.097  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .01           avgt  60   20.045  ±      0.102  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .10           avgt  60   20.667  ±      0.068  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .10           avgt  60   19.448  ±      0.126  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .50           avgt  60   13.217  ±      0.090  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .50           avgt  60   11.839  ±      0.042  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .90           avgt  60   7.404   ±      0.017  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .90           avgt  60   5.615   ±      0.009  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .99           avgt  60   5.517   ±      0.007  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .99           avgt  60   3.534   ±      0.009  ns/op

@cla-bot cla-bot bot added the cla-signed label Jan 27, 2022
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch from c3fff75 to 4b07829 Compare January 27, 2022 11:53
@sopel39
Copy link
Member

sopel39 commented Jan 27, 2022

Is it really a draft? If you think it's ready make it non-draft?

@radek-kondziolka radek-kondziolka marked this pull request as ready for review January 27, 2022 12:54
@sopel39
Copy link
Member

sopel39 commented Jan 31, 2022

mind automation (maven checks)

@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch 3 times, most recently from c3a8f3c to 45904d9 Compare February 8, 2022 09:59
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch 5 times, most recently from 130c1bd to 27e0015 Compare February 9, 2022 15:03
@sopel39 sopel39 changed the title Rk/dont use row block field block offsets Dont use row block field block offsets if not needed Feb 9, 2022
sopel39
sopel39 reviewed Feb 9, 2022
View reviewed changes
core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Show resolved Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Outdated Show resolved Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockEncoding.java Outdated Show resolved Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockEncoding.java Outdated
EncoderUtil.encodeNullsAsBits(sliceOutput, block);

if (isNullPresent && fieldBlockOffsets != null) {
int[] positions = IntStream.range(0, positionCount + 1).map(i -> fieldBlockOffsets[offsetBase + i] - startFieldBlockOffset).toArray();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

streams are not good in tight loops. You can use io.airlift.slice.Slices#wrappedIntArray(int[], int, int) to point at specific offset without copying data

core/trino-spi/src/test/java/io/trino/spi/block/TestRowBlock.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Outdated Show resolved Hide resolved
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch 2 times, most recently from ab279c6 to f48db07 Compare February 10, 2022 19:56
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch 3 times, most recently from 8d269bb to a163612 Compare February 11, 2022 10:26
@sopel39 sopel39 requested a review from skrzypo987 February 11, 2022 10:51
sopel39
sopel39 reviewed Feb 11, 2022
View reviewed changes
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Outdated Show resolved Hide resolved
testing/trino-tests/src/test/java/io/trino/execution/TestEventListenerWithSplits.java Show resolved Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockEncoding.java Outdated Show resolved Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/TestFromFieldBlocksCreatorForRowBlock.java Outdated
import static org.testng.Assert.assertNotNull;
import static org.testng.Assert.assertNull;

public class TestFromFieldBlocksCreatorForRowBlock
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

rename it back to testRowBlock

@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch 3 times, most recently from c4cd596 to 3fdc374 Compare February 11, 2022 12:22
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch 2 times, most recently from 4175e77 to 245c8d5 Compare February 11, 2022 12:24
skrzypo987
skrzypo987 reviewed Feb 15, 2022
View reviewed changes
Copy link
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added some comments, will look again tomoroow

core/trino-spi/src/main/java/io/trino/spi/block/AbstractRowBlock.java Show resolved Hide resolved
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch from 245c8d5 to afd150a Compare February 16, 2022 09:45
skrzypo987
skrzypo987 approved these changes Feb 17, 2022
View reviewed changes
Copy link
Member

@skrzypo987 skrzypo987 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM % comments

core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Outdated Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkDataGenerator.java Show resolved Hide resolved
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch from afd150a to d0ed9a0 Compare February 17, 2022 11:32
sopel39
sopel39 approved these changes Feb 22, 2022
View reviewed changes
Copy link
Member

@sopel39 sopel39 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

lgtm % comments

core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Show resolved Hide resolved
core/trino-main/src/test/java/io/trino/execution/buffer/BenchmarkBlockSerde.java Show resolved Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockEncoding.java Outdated Show resolved Hide resolved
core/trino-spi/src/main/java/io/trino/spi/block/RowBlockEncoding.java Outdated Show resolved Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/TestRowBlock.java Outdated Show resolved Hide resolved
core/trino-spi/src/test/java/io/trino/spi/block/TestRowBlock.java Outdated Show resolved Hide resolved
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch from d0ed9a0 to 2a2a3d5 Compare February 22, 2022 16:11
Do not use/serialize/deserialize fieldBlockOffset when not needed
f1459b2 
When there are no null rows within RowBlock
then it's not needed to store fieldBlockOffsets.
This improves performance and reduces
network traffic as fieldBlockOffsets
array is not serialized or deserialized.
Tests TestRowBlock and TestRowBlockEncoding were added.
Benchmark result:

Benchmark  (before/after)                      (nullChance)  Mode  Cnt  Score   Error  Units
(before)   BenchmarkBlockSerde.deserializeRow  0             avgt  60   17.348  ±      0.109  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  0             avgt  60   15.205  ±      0.084  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .01           avgt  60   18.020  ±      0.104  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .01           avgt  60   18.279  ±      0.113  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .10           avgt  60   16.859  ±      0.140  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .10           avgt  60   16.494  ±      0.157  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .50           avgt  60   10.273  ±      0.044  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .50           avgt  60   10.491  ±      0.063  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .90           avgt  60   4.351   ±      0.020  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .90           avgt  60   4.311   ±      0.013  ns/op
(before)   BenchmarkBlockSerde.deserializeRow  .99           avgt  60   2.919   ±      0.027  ns/op
(after)    BenchmarkBlockSerde.deserializeRow  .99           avgt  60   2.851   ±      0.008  ns/op
(before)   BenchmarkBlockSerde.serializeRow    0             avgt  60   20.862  ±      0.117  ns/op
(after)    BenchmarkBlockSerde.serializeRow    0             avgt  60   16.941  ±      0.089  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .01           avgt  60   21.588  ±      0.097  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .01           avgt  60   20.045  ±      0.102  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .10           avgt  60   20.667  ±      0.068  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .10           avgt  60   19.448  ±      0.126  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .50           avgt  60   13.217  ±      0.090  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .50           avgt  60   11.839  ±      0.042  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .90           avgt  60   7.404   ±      0.017  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .90           avgt  60   5.615   ±      0.009  ns/op
(before)   BenchmarkBlockSerde.serializeRow    .99           avgt  60   5.517   ±      0.007  ns/op
(after)    BenchmarkBlockSerde.serializeRow    .99           avgt  60   3.534   ±      0.009  ns/op
@radek-kondziolka radek-kondziolka force-pushed the rk/dont_use_RowBlock_fieldBlockOffsets branch from 2a2a3d5 to f1459b2 Compare February 22, 2022 20:38
@sopel39
Copy link
Member

sopel39 commented Feb 23, 2022

small comment

@sopel39 sopel39 merged commit bb60e2d into trinodb:master Feb 23, 2022
@sopel39
Copy link
Member

sopel39 commented Feb 23, 2022 •
edited
Loading

Too small, specific change for RN.

@mosabua mosabua mentioned this pull request Feb 23, 2022
Merged
@github-actions github-actions bot added this to the 372 milestone Feb 23, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@skrzypo987 skrzypo987 skrzypo987 approved these changes

@sopel39 sopel39 sopel39 approved these changes

@lukasz-stec lukasz-stec Awaiting requested review from lukasz-stec

Assignees
No one assigned
Labels
cla-signed
Milestone
372
Development

Successfully merging this pull request may close these issues.
3 participants
@radek-kondziolka @sopel39 @skrzypo987