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

Various fixes to the benchmark client and server #1944

Merged
merged 4 commits into from
Jun 21, 2024

Conversation

glbrntt
Copy link
Collaborator

@glbrntt glbrntt commented Jun 21, 2024

Motivation:

The benchmark client and server were implemented before we had a working transport making them impossible to test. As it turns out there were a few issues.

Modifications:

  • BenchmarkClient:
    • Some RPC types have been removed. All other implementations only support unary and streaming and we never run scenarios using the other types.
    • The implementation of the streaming RPC has been altered to record the latency per request/response message pair. This aligns with other implementations.
    • The implementation of the streaming RPC has also been changed to send the response message sequence into the request writer. This yields a fairly substantial performance improvement (~3.5x) over the existing implementation.
    • The streaming RPC now respects the messages per stream config being zero (meaning no limit).
    • An 'is shutting down' atomic is used to stop the client from initiating new RPCs before closing.
  • BenchmarkService:
    • No semantic changes; the typealiases have been desugared following from changes in fea1b72
  • WorkerService:
    • The state machine has been tightened up a bit to more clearly separate state from side effects and to avoid leaking the implementation of the state machine into the service.
    • Added logic for creating clients and servers with an HTTP/2 transport.

Result:

Can run perf tests

Motivation:

The benchmark client and server were implemented before we had a working
transport making them impossible to test. As it turns out there were a
few issues.

Modifications:

- BenchmarkClient:
  - Some RPC types have been removed. All other implementations only
    support unary and streaming and we never run scenarios using the
    other types.
  - The implementation of the streaming RPC has been altered to record
    the latency per request/response message pair. This aligns with
    other implementations.
  - The implementation of the streaming RPC has also been changed to
    send the response message sequence into the request writer. This
    yields a fairly substantial performance improvement (~3.5x) over
    the existing implementation.
  - The streaming RPC now respects the messages per stream config being
    zero (meaning no limit).
  - An 'is shutting down' atomic is used to stop the client from
    initiating new RPCs before closing.
- BenchmarkService:
  - No semantic changes; the typealiases have been desugared following
    from changes in fea1b72
- WorkerService:
  - The state machine has been tightened up a bit to more clearly
    separate state from side effects and to avoid leaking the
    implementation of the state machine into the service.
  - Added logic for creating clients and servers with an HTTP/2
    transport.

Result:

Can run perf tests
@glbrntt glbrntt requested a review from gjcairo June 21, 2024 08:59
@glbrntt glbrntt added the v2 A change for v2 label Jun 21, 2024
@glbrntt glbrntt requested a review from gjcairo June 21, 2024 11:16
@glbrntt glbrntt enabled auto-merge (squash) June 21, 2024 11:16
@glbrntt glbrntt merged commit cd00d5c into grpc:main Jun 21, 2024
17 checks passed
@glbrntt glbrntt deleted the perf-worker-fixes branch June 21, 2024 11:30
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
v2 A change for v2
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants