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

kvstreamer: perform more memory accounting #83683

Merged
merged 2 commits into from
Jul 6, 2022

Conversation

yuzefovich
Copy link
Member

kvstreamer: perform more memory accounting

This commit performs memory accounting for more of the internal state of
the streamer. In particular, it adds accounting for the overhead of
Results in the results buffer (previously, we would only account for
the size of the Get or the Scan response but not for the Result struct
itself). It also adds accounting for all slices that are O(N) in size
where N is the number of requests.

Addresses: #82160.

Release note: None

kvstreamer: account for the overhead of each request

Previously, we didn't account for some of the overhead of Get and Scan
requests, and this commit fixes that.

Here is the list of all things that we need to account for a single Get
request:

  • (if referenced by []roachpb.RequestUnion) the overhead of the
    roachpb.RequestUnion object
  • the overhead of roachpb.RequestUnion_Get object
  • the overhead of roachpb.GetRequest object
  • the footprint of the key inside of roachpb.GetRequest.

Previously, we only accounted for the first and the fourth items, and
now we account for all of them. Additionally, we used the
auto-generated Size method for the fourth item which I believe
represent the size of the serialized protobuf message (possibly
compressed - I'm not sure), but we're interested in the capacities of
the underlying slices.

Addresses: #82160.

Release note: None

@yuzefovich yuzefovich requested review from michae2 and a team June 30, 2022 23:16
@cockroach-teamcity
Copy link
Member

This change is Reviewable

This commit performs memory accounting for more of the internal state of
the streamer. In particular, it adds accounting for the overhead of
`Result`s in the results buffer (previously, we would only account for
the size of the Get or the Scan response but not for the `Result` struct
itself). It also adds accounting for all slices that are `O(N)` in size
where `N` is the number of requests.

Release note: None
Previously, we didn't account for some of the overhead of Get and Scan
requests, and this commit fixes that.

Here is the list of all things that we need to account for a single Get
request:
- (if referenced by `[]roachpb.RequestUnion`) the overhead of the
`roachpb.RequestUnion` object
- the overhead of `roachpb.RequestUnion_Get` object
- the overhead of `roachpb.GetRequest` object
- the footprint of the key inside of `roachpb.GetRequest`.

Previously, we only accounted for the first and the fourth items, and
now we account for all of them. Additionally, we used the
auto-generated `Size` method for the fourth item which I believe
represent the size of the serialized protobuf message (possibly
compressed - I'm not sure), but we're interested in the capacities of
the underlying slices.

Release note: None
@yuzefovich yuzefovich force-pushed the streamer-accounting branch from bb8d0ad to f6fcc6a Compare July 1, 2022 19:01
Copy link
Collaborator

@DrewKimball DrewKimball left a comment

Choose a reason for hiding this comment

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

:lgtm:

Reviewed 4 of 4 files at r1, 3 of 3 files at r2, all commit messages.
Reviewable status: :shipit: complete! 1 of 0 LGTMs obtained (waiting on @michae2)

@yuzefovich
Copy link
Member Author

TFTR!

bors r+

@craig
Copy link
Contributor

craig bot commented Jul 6, 2022

Build succeeded:

@craig craig bot merged commit 90b15cf into cockroachdb:master Jul 6, 2022
@yuzefovich yuzefovich deleted the streamer-accounting branch July 6, 2022 14:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants