Make 'GRPCStatus.Code' a struct #942
Merged
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Lukasa
approved these changes
Aug 18, 2020
Motivation: 'GRPCStatus.Code' came straight from the old gRPC Swift implementation, which in turn was copied almost verbatim from the grpc core library. It includes the case 'doNotUse' which is documented as existing to force users to include a default branch. Presumably, this is to ensure that cases can be added in the future without breaking API. Unfortunately that just doesn't cut it in Swift and we can enforce that a default branch is provide by using a struct. This also allows us to add additional status codes should any be added to the spec in the future. Modifications: - Turn `GRPCStatus.Code` into a `struct`, removing the `doNotUse` case - Add a deprecated `doNotUse` case to the shims - Add tests Result: - Programattically force a default branch when switching over 'GRPCStatus.Code'
glbrntt
changed the title
glbrntt
added a commit
to glbrntt/grpc-swift
that referenced
this pull request
Oct 12, 2020
Motivation: `GRPCStatus` is a `class` because it was too wide to fit within the inline storage of an existential container and would incur unnecessary heap allocations as a result. However, in grpc#942 the implementation of the `GRPCStatus.Code` stored by the status was changed from an `enum` to a `struct`. As a result, the width of `GRPCStatus` (as a `struct`) is now 24 and may therefore be stored inline in an existential container. Modifications: - Make `GRPCStatus` a `struct` instead of a `class` (note: we already have a test in place to validate it may be stored inline in an existential container). Result: - `GRPCStatus` no longer allocates
glbrntt
added a commit
to glbrntt/grpc-swift
that referenced
this pull request
Oct 13, 2020
Motivation: `GRPCStatus` is a `class` because it was too wide to fit within the inline storage of an existential container and would incur unnecessary heap allocations as a result. In grpc#942 the implementation of the `GRPCStatus.Code` stored by the status was changed from an `enum` to a `struct`. As a result, the width of `GRPCStatus` (as a `struct`) is now 24 and may therefore be stored inline in an existential container. Unfortunately, this now makes `_GRPCClientResponsePart` too wide! We could make the relevant case `indirect` on `_GRPClientResponsePart`, however since we transform between two specializations of that type per RPC (before and after deserialization) we would incur a heap allocation each time. Since the `message` property is optional, we can box that instead, only incurring the heap allocation when we construct a `GRPCStatus` with a non-nil status. Modifications: - Add `Refernce` and `Box` - Store the optional message in a `Box` - Make `GRPCStatus` a `struct` instead of a `class` (note: we already have a test in place to validate it may be stored inline in an existential container). Result: - `GRPCStatus` only allocates when the `message` isn't `nil`
glbrntt
added a commit
to glbrntt/grpc-swift
that referenced
this pull request
Oct 13, 2020
Motivation: `GRPCStatus` is a `class` because it was too wide to fit within the inline storage of an existential container and would incur unnecessary heap allocations as a result. In grpc#942 the implementation of the `GRPCStatus.Code` stored by the status was changed from an `enum` to a `struct`. As a result, the width of `GRPCStatus` (as a `struct`) is now 24 and may therefore be stored inline in an existential container. Unfortunately, this now makes `_GRPCClientResponsePart` too wide! This can be addressed by further narrowing the `Code` stored by the status. Modifications: - Narrow `GRPCStatus.Code` by storing its `rawValue` internally as a `UInt8` - Make `GRPCStatus` a `struct` instead of a `class` (note: we already have a test in place to validate it may be stored inline in an existential container). Result: - `GRPCStatus` only allocates when the `message` isn't `nil`
glbrntt
added a commit
to glbrntt/grpc-swift
that referenced
this pull request
Oct 13, 2020
Motivation: `GRPCStatus` is a `class` because it was too wide to fit within the inline storage of an existential container and would incur unnecessary heap allocations as a result. In grpc#942 the implementation of the `GRPCStatus.Code` stored by the status was changed from an `enum` to a `struct`. As a result, the width of `GRPCStatus` (as a `struct`) is now 24 and may therefore be stored inline in an existential container. Unfortunately, this now makes `_GRPCClientResponsePart` too wide! This can be addressed by further narrowing the `Code` stored by the status. Modifications: - Narrow `GRPCStatus.Code` by storing its `rawValue` internally as a `UInt8` - Make `GRPCStatus` a `struct` instead of a `class` (note: we already have a test in place to validate it may be stored inline in an existential container). Result: - `GRPCStatus` only allocates when the `message` isn't `nil`
glbrntt
added a commit
that referenced
this pull request
Oct 13, 2020
Motivation: `GRPCStatus` is a `class` because it was too wide to fit within the inline storage of an existential container and would incur unnecessary heap allocations as a result. In #942 the implementation of the `GRPCStatus.Code` stored by the status was changed from an `enum` to a `struct`. As a result, the width of `GRPCStatus` (as a `struct`) is now 24 and may therefore be stored inline in an existential container. Unfortunately, this now makes `_GRPCClientResponsePart` too wide! This can be addressed by further narrowing the `Code` stored by the status. Modifications: - Narrow `GRPCStatus.Code` by storing its `rawValue` internally as a `UInt8` - Make `GRPCStatus` a `struct` instead of a `class` (note: we already have a test in place to validate it may be stored inline in an existential container). Result: - `GRPCStatus` only allocates when the `message` isn't `nil`
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Motivation:
'GRPCStatus.Code' came straight from the old gRPC Swift implementation,
which in turn was copied almost verbatim from the grpc core library. It
includes the case 'doNotUse' which is documented as existing to force
users to include a default branch. Presumably, this is to ensure that
cases can be added in the future without breaking API. Unfortunately
that just doesn't cut it in Swift and we can enforce that a default
branch is provide by using a struct. This also allows us to add
additional status codes should any be added to the spec in the future.
Modifications:
GRPCStatus.Codeinto astruct, removing thedoNotUsecasedoNotUsecase to the shimsResult:
'GRPCStatus.Code'