TrueCloudLab/frostfs-api-go

Fork 16

Use `protowire` in `util/proto` helpers #50

New issue

Closed

opened 2023-07-26 15:35:08 +00:00 by fyrchik · 5 comments

fyrchik commented

2023-07-26 15:35:08 +00:00

Owner

It has almost everything we need https://pkg.go.dev/google.golang.org/protobuf@v1.31.0/encoding/protowire#SizeVarint
And communicates our intentions somewhat more clearly. Also, helps to get rid of all these << | bit trickery.

It has almost everything we need https://pkg.go.dev/google.golang.org/protobuf@v1.31.0/encoding/protowire#SizeVarint And communicates our intentions somewhat more clearly. Also, helps to get rid of all these `<< |` bit trickery.

aarifullin self-assigned this 2023-08-02 07:32:56 +00:00

fyrchik commented

2023-08-02 09:34:58 +00:00

Author

Owner

After this we can change protogen and, possibly, replace api-go dependency with protowire

After this we can change `protogen` and, possibly, replace `api-go` dependency with `protowire`

aarifullin referenced this issue

2023-08-02 13:55:44 +00:00

[#50] util: Use protowire for util/proto helpers #52

fyrchik referenced this issue from a commit

2023-08-10 16:40:56 +00:00

[#50] util: Use protowire for util/proto helpers

aarifullin closed this issue

2023-08-11 07:19:18 +00:00

fyrchik reopened this issue

2023-08-11 07:34:32 +00:00

fyrchik commented

2023-08-11 07:34:59 +00:00

Author

Owner

We still have a bunch of opportunities for protowire.AppendBytes etc.
Let's benchmark and replace if possible.

We still have a bunch of opportunities for `protowire.AppendBytes` etc. Let's benchmark and replace if possible.

aarifullin commented

2023-08-11 08:15:05 +00:00

Member

We still have a bunch of opportunities for protowire.AppendBytes etc.
Let's benchmark and replace if possible.

Ah, ok! But we need take in count that it requires some refactoring for StableMarshaller interface to make it possible to use AppendBytes

> We still have a bunch of opportunities for `protowire.AppendBytes` etc. > Let's benchmark and replace if possible. Ah, ok! But we need take in count that it requires some refactoring for StableMarshaller interface to make it possible to use `AppendBytes`

fyrchik commented

2023-08-11 09:08:36 +00:00

Author

Owner

Not necessary: copy(a, b) can be replaced with append(a[:0], b...), but let's check whether it is slower.

Not necessary: `copy(a, b)` can be replaced with `append(a[:0], b...)`, but let's check whether it is slower.

aarifullin referenced this issue

2023-08-31 09:15:59 +00:00

WIP: [#50] marshal: Use protowire append methods for marshalers #56

aarifullin commented

2023-08-31 15:48:43 +00:00

Member

The pull-request #56 demonstrates why using protowire.Append... methods is not good idea.

All api-defined types must impelement StableMarshal([]byte) []byte) method (to implement StableMarshaller interface)

When a primitive numeric type is going to be serialized, these methods are used:

UInt64Marshal
Int64Marshal
...

As you can see, these methods return the offset.

If we consider the idea of using StableMarshal, then we can see that we should allocate a slice with defined length (make([]byte, N) and then we shift the offset for each internal StableMarshal invocation (io.Write behaves in the same way)

protowire.Append... method oblige us to allocate a slice with defined capacity with 0 length (make([]byte, 0, N). So, when we go down to UInt64Marshal or Int64Marshal we need to make these methods return the slice instead the offset. Such refactoring breaks frostfs-node because there are methods for protobufs that are generated by protoc plugin.

We want to keep the same interface and support backward compability. However, another good idea comes up - we may refactor StableMarshal to Write and make types implement io.Writer interface that can be very useful

The pull-request [#56](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/pulls/56) demonstrates why using `protowire.Append...` methods is not good idea. All api-defined types must impelement `StableMarshal([]byte) []byte)` method (to implement `StableMarshaller` interface) When a primitive numeric type is going to be serialized, these methods are used: 1. [UInt64Marshal](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/util/proto/marshal.go#L94) 2. [Int64Marshal](https://git.frostfs.info/TrueCloudLab/frostfs-api-go/src/branch/master/util/proto/marshal.go#L116) ... As you can see, these methods return **the offset**. If we consider the idea of using `StableMarshal`, then we can see that we should allocate a slice with defined **length** (`make([]byte, N`) and then we shift the offset for each internal `StableMarshal` invocation (`io.Write` behaves in the same way) `protowire.Append...` method oblige us to allocate a slice with defined capacity with 0 length (`make([]byte, 0, N`). So, when we go down to `UInt64Marshal` or `Int64Marshal` we need to make these methods return the slice instead the offset. Such refactoring breaks `frostfs-node` because there are methods for protobufs that are generated by protoc plugin. We want to keep the same interface and support backward compability. However, another good idea comes up - we may refactor `StableMarshal` to `Write` and make types implement `io.Writer` interface that can be very useful

aarifullin closed this issue

2023-08-31 15:48:43 +00:00