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

Reduce allocations in the GRPCClientStateMachine #653

Merged
merged 2 commits into from Dec 11, 2019

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Dec 11, 2019

Motivation:

There were some unnecessary allocations in the client state machine.

Modifications:

  • Use HPACKHeaders new first(name:) method to avoid allocating an
    array of headers before taking the first.
  • Turn LengthPrefixedMessageWriter into a struct.
  • Turn LengthPrefixedMessageReader into a struct.

Result:

  • Reduce allocations, a small perf gain.

Motivation:

There were some unnecessary allocations in the client state machine.

Modifications:

- Use `HPACKHeader`s new `first(name:)` method to avoid allocating an
  array of headers before taking the first.
- Turn LengthPrefixedMessageWriter into a struct.
- Turn LengthPrefixedMessageReader into a struct.

Result:

- Reduce allocations, a small perf gain.
@glbrntt glbrntt requested a review from MrMage December 11, 2019 10:44
@glbrntt
Copy link
Collaborator Author

glbrntt commented Dec 11, 2019

Before:

  • embedded_client_unary_10k_small_requests: 372,371,354,371,379,371,372,375,385,381

After:

  • embedded_client_unary_10k_small_requests: 341,340,343,345,343,342,344,354,352,340

@glbrntt
Copy link
Collaborator Author

glbrntt commented Dec 11, 2019

I also had more targeted benchmark for just the state machine (which involved making a bunch of things public) which saw a decrease from ~60ms to ~50ms.

Copy link
Collaborator

@MrMage MrMage left a comment

Choose a reason for hiding this comment

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

Cool stuff! Suggested a few more block replacements.

@@ -544,21 +544,20 @@ extension GRPCClientStateMachine.State {
// responses as well as a variety of non-GRPC content-types and to omit Status & Status-Message.
// Implementations must synthesize a Status & Status-Message to propagate to the application
// layer when this occurs."
let statusHeader = headers[":status"].first
let responseStatus = statusHeader.flatMap(Int.init).map { code in
let responseStatus = headers.first(name: ":status").flatMap(Int.init).map { code in
HTTPResponseStatus(statusCode: code)
Copy link
Collaborator

Choose a reason for hiding this comment

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

.map(HTTPResponseStatus.init)?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Unfortunately the init has a default second arg: init(statusCode: Int, reasonPhrase: String = "") so this isn't possible.

Sources/GRPC/GRPCClientStateMachine.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCClientStateMachine.swift Outdated Show resolved Hide resolved
Sources/GRPC/GRPCClientStateMachine.swift Show resolved Hide resolved
@MrMage
Copy link
Collaborator

MrMage commented Dec 11, 2019

I also had more targeted benchmark for just the state machine (which involved making a bunch of things public) which saw a decrease from ~60ms to ~50ms.

We could make those public if we declare them as not part of the public API.

@glbrntt
Copy link
Collaborator Author

glbrntt commented Dec 11, 2019

I also had more targeted benchmark for just the state machine (which involved making a bunch of things public) which saw a decrease from ~60ms to ~50ms.

We could make those public if we declare them as not part of the public API.

True, I think I'll leave that for another PR though!

@glbrntt glbrntt merged commit 3111de6 into grpc:nio Dec 11, 2019
@glbrntt glbrntt deleted the gb-benchmarks-state-machine branch December 11, 2019 11:54
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.

None yet

2 participants