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

Avoid unnecessary arrays. #935

Merged
merged 1 commit into from
Aug 12, 2020
Merged

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