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.

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

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

Closed
lukasz-stec opened this issue May 20, 2022 · 4 comments · Fixed by #12512
Closed

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

lukasz-stec opened this issue May 20, 2022 · 4 comments · Fixed by #12512

Comments

@lukasz-stec
Copy link
Member

lukasz-stec commented May 20, 2022

Follow up after #12336

For fixed width type you can assume here that there will be OUTPUT_PAGE_SIZE positions (type check on instaceof FixedWidthType.
For variable length type, (instanceof VariableWidthType) you can assume OUTPUT_PAGE_SIZE positions, but can also assume some skew via io.trino.spi.block.BlockUtil#calculateBlockResetBytes. You can then use io.trino.spi.type.Type#createBlockBuilder(io.trino.spi.block.BlockBuilderStatus, int, int) to construct builder of right size.

This should save memory compared to existing code. I wouldn't be surprised to see aggregation mem usage drop by 10-20%

I think we can extend io.trino.spi.PageBuilder#reset to accept expectedEntries.

https://github.com/trinodb/trino/pull/12072/files#r861792201

@lukasz-stec
Copy link
Member Author

cc @sopel39 @skrzypo987

@skrzypo987
Copy link
Member

Isn't this a one-liner after #12336 lands?

@lukasz-stec
Copy link
Member Author

Isn't this a one-liner after #12336 lands?

@skrzypo987 No, the issue is that VariableWIdthBlockBuilder needs to be reset/recreated with a similar (currently 25% higher) expectedBytes. So we either do something as I did in #12512 or we need to expose

VariableWidthBlockBuilder.getCurrentSizeInBytes()
{
  return getOffset(positions) - getOffset(0)
}

@skrzypo987
Copy link
Member

ok, I got you.
Still this does not seem too complicated.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Development

Successfully merging a pull request may close this issue.

3 participants