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

Fix unnecessary slice moving in slice_buffer.c:maybe_embiggen #10211

Merged
merged 2 commits into from
Apr 7, 2017

Conversation

muxi
Copy link
Member

@muxi muxi commented Mar 17, 2017

Avoid moving slices unnecessarily when invoking grpc_slice_buffer_add that impacted performance.

Cherry-picked from #9626 as it is still going to take some time to submit that PR, while this change is orthogonal and going to benefit widely.

} else {
if (sb->base_slices != sb->slices) {
Copy link
Contributor

Choose a reason for hiding this comment

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

It would be good to do this before growing the capacity: i.e line 54 above sb->capacity = GROW(sb->capacity)
Because after the memmove, we might have some space and no longer have to do a gpr_realloc

So perhaps it would be just good do the memmove just before checking for if (slice_count == sb->capacity)..

i.e something like:

 static void maybe_embiggen(grpc_slice_buffer *sb) {
    size_t slice_offset = (size_t)(sb->slices - sb->base_slices);
    size_t slice_count = sb->count + slice_offset;
  
    if (slice_count == sb->capacity && sb->base_slices != sb->slices) {
      /* do the memmove */
      /* re-calculate slice_offset or rather just reset slice_count to sb->count */
    }
    ..
}

Copy link
Member Author

Choose a reason for hiding this comment

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

That sounds great.

Copy link
Contributor

@sreecha sreecha left a comment

Choose a reason for hiding this comment

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

This looks good. Thanks Muxi.

@muxi
Copy link
Member Author

muxi commented Apr 6, 2017

test this please

@grpc-kokoro
Copy link

No significant performance differences

@muxi
Copy link
Member Author

muxi commented Apr 7, 2017

gRPC_interop_pull_requests - #9011

@muxi muxi merged commit dd550c7 into grpc:master Apr 7, 2017
@muxi muxi deleted the fix-maybe-embiggen branch April 7, 2017 16:47
@lock lock bot locked as resolved and limited conversation to collaborators Jan 23, 2019
@lock lock bot unassigned sreecha Jan 23, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants