Reset MultiChannelGroupByHash.currentPageBuilder to exact position count #12512

lukasz-stec · 2022-05-23T07:41:17Z

Description

Since MultiChannelGroupByHash knows exactly how many
positions currentPageBuilder can eventually contain,
there is no need to increase the size with
calculateBlockResetSize, wasting memory in the process.

This fixes #12484
It's based on #12336. Only last commit matters here.

tpch/tpcds benchmark

There is a 1-2 % improvement in peak memory consumption.

reset-exeact-orc-part-sf1000.pdf

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

improvement

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

core query engine, spi BlockBuilder

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

Lower hash aggregation memory consumption

Related issues, pull requests, and links

Fixes Presize new page to exeact expect size in MultiChannelGroupByHash#startNewPage #12484

Documentation

( ) 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.
( ) Release notes entries required with the following suggested text:

# Section
* Fix some things. ({issue}`issuenumber`)

lukasz-stec · 2022-05-24T09:20:55Z

io.trino.spi.TestSpiBackwardCompatibility.testSpiSingleVersionBackwardCompatibility fails becasue BlockBuilder newBlockBuilderLike(BlockBuilderStatus blockBuilderStatus) was implemented in the BlockBuilder interface as a default method and this test does not support that.
IF we go with this PR, we will need to rework TestSpiBackwardCompatibility probably

lukasz-stec · 2022-06-28T13:17:54Z

if we decide to go ahead with this change, io.trino.spi.TestSpiBackwardCompatibility.testSpiSingleVersionBackwardCompatibility needs to be fixed

core/trino-spi/src/main/java/io/trino/spi/block/VariableWidthBlockBuilder.java

core/trino-spi/src/main/java/io/trino/spi/block/BlockBuilder.java

sopel39 · 2022-07-15T14:05:03Z

core/trino-spi/src/main/java/io/trino/spi/PageBuilder.java

@@ -98,6 +98,19 @@ public void reset()
        }
    }

+    public void resetExact()


Add reset(int expectedEntries) method instead.

Exact is ambiguous. Exact what? positions? size?

90%* of method names are ambiguous (see no further than newBlockBuilderLike).
I think keeping the method without expectedEntries param is better as we don't need the extended functionality of resetting PageBuilder to any position count.

made up stat

I agree with Łukasz. The name is not that bad, and it will only be invoked with the same argument every time. Some simple javadoc might be added though

Definitely needs a javadoc
Alternatively, how about a PageBuilder newPageBuilderLike(int expectedEntries) method instead ?

to avoid the Javadoc but also avoid exposing unnecessary functionality I added private
private void reset(int expectedEntries) and public resetExact with an extremely straightforward implementation that does not need documentation.

reset(int expectedEntries) may correspond to the PageBuilder constructor but it's not needed, why add it to the public API?

How about instead of inlining resetExact we change the name to be less ambiguous? The approach of inlining method calls because we don't like the method name is not good long term.

I'm not sure you can inline it TBH due to page builder status

ping 9e984ab#r950126334

@sopel39 see https://github.com/trinodb/trino/pull/12512/files#r950167250
to sum up. we don't need reset(int expectedEntries) public API, we need resetExact. If the name is misleading please propose a better name but I stand by the fact that resetExact captures the semantics we need and reset(int expectedEntries) adds complexity.

in order to move forward with the PR I dropped the resetExact and made void reset(int expectedEntries) public.

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

lukasz-stec

rebased on the master + renamed newBlockBuilderLike param

core/trino-main/src/main/java/io/trino/operator/MultiChannelGroupByHash.java

lukasz-stec · 2022-08-12T09:06:20Z

core/trino-spi/src/main/java/io/trino/spi/PageBuilder.java

@@ -98,6 +98,19 @@ public void reset()
        }
    }

+    public void resetExact()


90%* of method names are ambiguous (see no further than newBlockBuilderLike).
I think keeping the method without expectedEntries param is better as we don't need the extended functionality of resetting PageBuilder to any position count.

made up stat

lukasz-stec · 2022-08-12T09:40:31Z

this change adds new method to BlockBuilder in SPI so it's not backward compatible (and TestSpiBackwardCompatibility tests fail for that reason https://github.com/trinodb/trino/runs/7087416672?check_suite_focus=true).
@sopel39 @kokosing do you know how we deal with this kind of change?

skrzypo987

lgtm.

Since the pages considered her are temporary (not the output ones), consider making them fixed size from the beginning (let's say of size 1024). That way the value lookup consists of a bit shift and a single memory lookup instead of two. That is just me thinking out loud, you can consider this out of scope.

skrzypo987 · 2022-08-17T04:49:57Z

core/trino-spi/src/main/java/io/trino/spi/PageBuilder.java

@@ -98,6 +98,19 @@ public void reset()
        }
    }

+    public void resetExact()


I agree with Łukasz. The name is not that bad, and it will only be invoked with the same argument every time. Some simple javadoc might be added though

raunaqmorarka

lgtm % comment.
Also, PTAL if GeneratedPageProjection#project can benefit from similar exact sizing

raunaqmorarka · 2022-08-17T07:51:16Z

core/trino-spi/src/main/java/io/trino/spi/PageBuilder.java

@@ -98,6 +98,19 @@ public void reset()
        }
    }

+    public void resetExact()


Definitely needs a javadoc
Alternatively, how about a PageBuilder newPageBuilderLike(int expectedEntries) method instead ?

lukasz-stec · 2022-08-17T09:44:27Z

PTAL if GeneratedPageProjection#project can benefit from similar exact sizing

Not an expert in projection but it looks like this could indeed benefit from the exact sizing. I can do this as I follow up on this PR.

lukasz-stec · 2022-08-17T09:47:02Z

Since the pages considered her are temporary (not the output ones), consider making them fixed size from the beginning (let's say of size 1024).

@skrzypo987 I think this could have negative consequences if the number of groups is small

raunaqmorarka · 2022-08-17T13:29:59Z

@lukasz-stec PTAL at test failures

lukasz-stec · 2022-08-17T13:43:56Z

PTAL at test failures

@raunaqmorarka TestSpiBackwardCompatibility is failing. I'm not sure how to proceed.
See #12512 (comment).
This of course needs to be fixed before we can merge this.

raunaqmorarka · 2022-08-18T10:55:44Z

PTAL at test failures

@raunaqmorarka TestSpiBackwardCompatibility is failing. I'm not sure how to proceed. See #12512 (comment). This of course needs to be fixed before we can merge this.

I think we need to keep the existing API around as well to keep it backward compatible. Maybe mark it Deprecated.

lukasz-stec · 2022-08-18T12:34:28Z

I think we need to keep the existing API around as well to keep it backward compatible. Maybe mark it Deprecated.

The existing API is still there but now it's implemented as a default interface method.
TestSpiBackwardCompatibility does not handle changes like that

raunaqmorarka · 2022-08-18T12:40:01Z

I think we need to keep the existing API around as well to keep it backward compatible. Maybe mark it Deprecated.

The existing API is still there but now it's implemented as a default interface method. TestSpiBackwardCompatibility does not handle changes like that

If it's not going to require changes to any existing usage (i.e. it's not really backward incompatible change), then we can probably just add the changes to the exceptions list with a comment to get past this.

lukasz-stec · 2022-08-29T11:23:50Z

rebased on the master + added TestSpiBackwardCompatibility exceptions for the newBlockBuilderLike method

core/trino-spi/src/main/java/io/trino/spi/block/MapBlockBuilder.java

sopel39 · 2022-08-31T08:08:57Z

@electrum landed API verifier, please rebase and make sure tests pass

lukasz-stec · 2022-08-31T08:52:31Z

@electrum landed API verifier, please rebase and make sure tests pass

rebased and added an exception to the revapi config.

Since MultiChannelGroupByHash knows exactly how many positions currentPageBuilder can eventually contain, there is no need to increase the size with calculateBlockResetSize, wasting memory in the process.

cla-bot bot added the cla-signed label May 23, 2022

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from fccc31f to 3326feb Compare May 23, 2022 09:14

lukasz-stec mentioned this pull request May 24, 2022

Presize new page to exeact expect size in MultiChannelGroupByHash#startNewPage #12484

Closed

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from 3326feb to fbdf135 Compare June 28, 2022 06:59

lukasz-stec added the performance label Jun 28, 2022

lukasz-stec marked this pull request as ready for review June 28, 2022 13:16

lukasz-stec requested a review from sopel39 June 28, 2022 13:16

sopel39 reviewed Jul 15, 2022

View reviewed changes

sopel39 requested a review from gaurav8297 August 2, 2022 10:30

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from fbdf135 to a2e608a Compare August 12, 2022 09:10

lukasz-stec commented Aug 12, 2022

View reviewed changes

lukasz-stec requested a review from sopel39 August 12, 2022 09:21

lukasz-stec requested a review from skrzypo987 August 16, 2022 10:43

skrzypo987 approved these changes Aug 17, 2022

View reviewed changes

raunaqmorarka reviewed Aug 17, 2022

View reviewed changes

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from a2e608a to 9b54b1a Compare August 17, 2022 09:45

lukasz-stec requested a review from raunaqmorarka August 17, 2022 09:45

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from 9b54b1a to 2706561 Compare August 17, 2022 10:26

raunaqmorarka approved these changes Aug 17, 2022

View reviewed changes

lukasz-stec mentioned this pull request Aug 23, 2022

Allow method move to a superclass in TestSpiBackwardCompatibility #13797

Closed

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch 2 times, most recently from be20719 to 9e984ab Compare August 29, 2022 11:23

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch 2 times, most recently from 55566f2 to 18e1c53 Compare August 30, 2022 12:19

sopel39 approved these changes Aug 30, 2022

View reviewed changes

sopel39 reviewed Aug 30, 2022

View reviewed changes

core/trino-spi/src/main/java/io/trino/spi/block/MapBlockBuilder.java Outdated Show resolved Hide resolved

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from 18e1c53 to 0699a68 Compare August 31, 2022 08:50

Reset page builder to exact position count

4a971e9

Since MultiChannelGroupByHash knows exactly how many positions currentPageBuilder can eventually contain, there is no need to increase the size with calculateBlockResetSize, wasting memory in the process.

lukasz-stec force-pushed the ls/017-mcgbh-reset-precise branch from 0699a68 to 4a971e9 Compare August 31, 2022 09:25

sopel39 merged commit aee2179 into trinodb:master Aug 31, 2022

sopel39 mentioned this pull request Aug 31, 2022

Release notes for 395 #13913

Closed

github-actions bot added this to the 395 milestone Aug 31, 2022

colebow mentioned this pull request Sep 2, 2022

Add Trino 395 release notes #13975

Merged

lukasz-stec mentioned this pull request Sep 27, 2022

Pre-size projection BlockBuilder to the exact size needed #14309

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reset MultiChannelGroupByHash.currentPageBuilder to exact position count #12512

Reset MultiChannelGroupByHash.currentPageBuilder to exact position count #12512

lukasz-stec commented May 23, 2022 •

edited

lukasz-stec commented May 24, 2022

lukasz-stec commented Jun 28, 2022

sopel39 Jul 15, 2022

lukasz-stec Aug 12, 2022

skrzypo987 Aug 17, 2022

raunaqmorarka Aug 17, 2022

lukasz-stec Aug 17, 2022

lukasz-stec Aug 19, 2022

sopel39 Aug 19, 2022

sopel39 Aug 29, 2022

lukasz-stec Aug 29, 2022

lukasz-stec Aug 30, 2022

lukasz-stec left a comment

lukasz-stec Aug 12, 2022

lukasz-stec commented Aug 12, 2022

skrzypo987 left a comment

skrzypo987 Aug 17, 2022

raunaqmorarka left a comment

raunaqmorarka Aug 17, 2022

lukasz-stec commented Aug 17, 2022

lukasz-stec commented Aug 17, 2022

raunaqmorarka commented Aug 17, 2022

lukasz-stec commented Aug 17, 2022

raunaqmorarka commented Aug 18, 2022

lukasz-stec commented Aug 18, 2022

raunaqmorarka commented Aug 18, 2022

lukasz-stec commented Aug 29, 2022

sopel39 commented Aug 31, 2022

lukasz-stec commented Aug 31, 2022

Reset MultiChannelGroupByHash.currentPageBuilder to exact position count #12512

Reset MultiChannelGroupByHash.currentPageBuilder to exact position count #12512

Conversation

lukasz-stec commented May 23, 2022 • edited

Description

tpch/tpcds benchmark

Related issues, pull requests, and links

Documentation

Release notes

lukasz-stec commented May 24, 2022

lukasz-stec commented Jun 28, 2022

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec commented Aug 12, 2022

skrzypo987 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

raunaqmorarka left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

lukasz-stec commented Aug 17, 2022

lukasz-stec commented Aug 17, 2022

raunaqmorarka commented Aug 17, 2022

lukasz-stec commented Aug 17, 2022

raunaqmorarka commented Aug 18, 2022

lukasz-stec commented Aug 18, 2022

raunaqmorarka commented Aug 18, 2022

lukasz-stec commented Aug 29, 2022

sopel39 commented Aug 31, 2022

lukasz-stec commented Aug 31, 2022

lukasz-stec commented May 23, 2022 •

edited