Skip to content

Fix retained size after Page deserialization#16248

Closed
viczhang861 wants to merge 1 commit intoprestodb:masterfrom
viczhang861:block_view_page
Closed

Fix retained size after Page deserialization#16248
viczhang861 wants to merge 1 commit intoprestodb:masterfrom
viczhang861:block_view_page

Conversation

@viczhang861
Copy link
Contributor

@viczhang861 viczhang861 commented Jun 11, 2021

Slice in SerializedPage includes all blocks. When VariableWidthBlock
is deserialized using VariableWidthBlockEncoding, VariableWidthBlock
should be backed by a new slice with copied data rather than the
slice for all blocks.

Otherwise, every block holds a slice of the entire page, which leads to
wrong calculation of Page retained size. Depends on number of affected
blocks in a page, memory could be off by N times (N >= count of VariableWidthBlock).
As a result, spilled query will have incorrect retained size and fail.

Test plan

  • Tested in production, previously OOMed spilled query succeeds after this change.

  • Performance regression test
    612 production queries (CPU time >= 1 hour) are run with and without this patch. Total cpu hours is 24680 for test job and 23956 for control run.

== NO RELEASE NOTE ==

@viczhang861 viczhang861 requested a review from a team June 11, 2021 07:01
Slice in SerializedPage includes all blocks. When VariableWidthBlock
is deserilized using VariableWidthBlockEncoding, VariableWidthBlock
should be backed by a new slice with copied data rather than a view
of the slice with all blocks.
@pettyjamesm
Copy link
Contributor

I would love to avoid an extra copy for VariableWidthBlock instances here, but since VariableWidthBlockEncoding is the only block encoding I'm aware of that creates direct references to the input slice instead of copying data out into newly allocated primitive arrays it definitely makes things more consistent and of course addresses the retained size issue. Do we have a reasonable way to quantify the performance trade-off involved with making this change?

@stale
Copy link

stale bot commented Apr 16, 2022

This pull request has been automatically marked as stale because it has not had recent activity. If you'd still like this PR merged, please comment on the task, make sure you've addressed reviewer comments, and rebase on the latest master. Thank you for your contributions!

@stale stale bot added the stale label Apr 16, 2022
@stale stale bot closed this Apr 28, 2022
@rohanpednekar
Copy link
Contributor

Hi @viczhang861 , Do you think you can get this fixed? We would like to get it reviewed and merged. Please let me know if you need any additional help. Thanks!

@rschlussel
Copy link
Contributor

Hi @viczhang861 , Do you think you can get this fixed? We would like to get it reviewed and merged. Please let me know if you need any additional help. Thanks!

we fixed this issue with #17641

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

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants