Skip to content

Implement a new code generator that utilizes google.golang.org/protobuf#91

Closed
kzys wants to merge 5 commits intocontainerd:mainfrom
kzys:new-protogen
Closed

Implement a new code generator that utilizes google.golang.org/protobuf#91
kzys wants to merge 5 commits intocontainerd:mainfrom
kzys:new-protogen

Conversation

@kzys
Copy link
Copy Markdown
Member

@kzys kzys commented Aug 20, 2021

A PoC for #92.

@kzys kzys changed the title Add protoc-gen-go-ttrpc to remove github.com/gogo/protobuf Implement a new code generator that utilizes google.golang.org/protobuf Aug 20, 2021
@kzys kzys marked this pull request as draft August 20, 2021 23:08
@kzys kzys force-pushed the new-protogen branch 4 times, most recently from 86d82f7 to bb30fa5 Compare August 21, 2021 00:18
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@kzys kzys force-pushed the new-protogen branch 2 times, most recently from 44de7e4 to a2db96b Compare August 21, 2021 03:28
protobuild is currently not compatible with the latest protoc.

Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
@codecov-commenter
Copy link
Copy Markdown

codecov-commenter commented Aug 21, 2021

Codecov Report

Merging #91 (be9f624) into main (360e86c) will increase coverage by 2.73%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##             main      #91      +/-   ##
==========================================
+ Coverage   70.75%   73.48%   +2.73%     
==========================================
  Files          11       12       +1     
  Lines         612      728     +116     
==========================================
+ Hits          433      535     +102     
- Misses        142      155      +13     
- Partials       37       38       +1     
Impacted Files Coverage Δ
client.go 84.53% <ø> (+3.00%) ⬆️
vtcodec.go 55.55% <55.55%> (ø)
codec.go 71.42% <100.00%> (+11.42%) ⬆️
services.go 47.36% <100.00%> (+5.57%) ⬆️
handshake.go 100.00% <0.00%> (ø)
interceptor.go 100.00% <0.00%> (ø)
server.go 76.19% <0.00%> (+1.70%) ⬆️
metadata.go 93.02% <0.00%> (+2.39%) ⬆️
channel.go 84.31% <0.00%> (+2.49%) ⬆️
... and 4 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 360e86c...be9f624. Read the comment docs.

Kazuyoshi Kato added 2 commits August 20, 2021 20:36
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Signed-off-by: Kazuyoshi Kato <katokazu@amazon.com>
Comment thread codec.go
var proto encoding.Codec

func init() {
proto = &vtcodec{vt:&vtproto.Codec{}}
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Where does this get used?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

The vtcodec type is needed to invoke vtprotobuf's marshal/unmarshal code.

Comment thread example/example.pb.go
if protoimpl.UnsafeEnabled && x != nil {
ms := protoimpl.X.MessageStateOf(protoimpl.Pointer(x))
if ms.LoadMessageInfo() == nil {
ms.StoreMessageInfo(mi)
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I am not sure it matters for ttrpc's use case, but are we going to be taking a performance regression?

Copy link
Copy Markdown
Member Author

Choose a reason for hiding this comment

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

I'm not really sure how that is used. Let me take a look next week.

func soz(x uint64) (n int) {
return sov(uint64((x << 1) ^ uint64((int64(x) >> 63))))
}
func (m *Method1Request) UnmarshalVT(dAtA []byte) error {
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Ah, I see. VT has their own separate interface and methods for using unrolled marshal/unmarshal.

Comment thread protobuild.sh
# See the License for the specific language governing permissions and
# limitations under the License.

set -euxo pipefail
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

I know this is temporary, but we might want to start with protobuild support to get it working with modules. The technique will require a copy of the import set, but I am not sure how that will work with --import_path going away.

@kzys
Copy link
Copy Markdown
Member Author

kzys commented Oct 26, 2021

Resolving. I'm going to spin out small easy-to-merge PRs instead. The first PR is #96.

@kzys kzys closed this Oct 26, 2021
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.

4 participants