Skip to content

Conversation

@Lukasa
Copy link
Collaborator

@Lukasa Lukasa commented Aug 12, 2020

Motivation:

HTTP1ToGRPCServerCodec currently creates a temporary array for parsing
all messages into before it forwards them on. This is both a minor perf
drain (due to the extra allocations) and a correctness problem, as it
makes this channel handler non-reentrant-safe. We should fix both
issues.

Modifications:

  • Replace the temporary array with a simple loop.
  • Add tests that validates correct behaviour on reentrancy.

Result:

Better re-entrancy behaviour! Verrrry slightly better perf.

@Lukasa Lukasa requested a review from glbrntt August 12, 2020 14:30
@Lukasa Lukasa force-pushed the cb-avoid-allocating-arrays branch 2 times, most recently from 9c51548 to ea4b679 Compare August 12, 2020 15:14
Copy link
Collaborator

@glbrntt glbrntt left a comment

Choose a reason for hiding this comment

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

Looks great but you also need to update the test manifests...? (I need to bring this forward as part of the sanity check)

Motivation:

HTTP1ToGRPCServerCodec currently creates a temporary array for parsing
all messages into before it forwards them on. This is both a minor perf
drain (due to the extra allocations) and a correctness problem, as it
makes this channel handler non-reentrant-safe. We should fix both
issues.

Modifications:

- Replace the temporary array with a simple loop.
- Add tests that validates correct behaviour on reentrancy.

Result:

Better re-entrancy behaviour! Verrrry slightly better perf.
@Lukasa Lukasa force-pushed the cb-avoid-allocating-arrays branch from ea4b679 to 302209f Compare August 12, 2020 15:25
@Lukasa
Copy link
Collaborator Author

Lukasa commented Aug 12, 2020

Updated

@glbrntt glbrntt merged commit d3f039d into grpc:main Aug 12, 2020
@glbrntt glbrntt added the 🔨 semver/patch No public API change. label Aug 27, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

🔨 semver/patch No public API change.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants