gogo / protobuf

[Deprecated] Protocol Buffers for Go with Gadgets
Other
5.67k stars 807 forks source link

Less pointers for repeated fields #309

Closed Civil closed 7 years ago

Civil commented 7 years ago

Maybe I'm missing something, but I can't see flag that will allow me to make repeated field to be unmarshaled to slice of structs, not to slice of pointers.

Example:

message Point {
  uint32 timestamp = 1;
  double value = 2;
}

message Metric {
  string metric = 1;
  repeated Point points = 2;
}

This will result in following structs:

type Point struct {
    Timestamp uint32  `protobuf:"varint,1,opt,name=timestamp,proto3" json:"timestamp,omitempty"`
    Value     float64 `protobuf:"fixed64,2,opt,name=value,proto3" json:"value,omitempty"`
}

type Metric struct {
    Metric string  `protobuf:"bytes,1,opt,name=metric,proto3" json:"metric,omitempty"`
    Points []*Point `protobuf:"bytes,2,rep,name=points" json:"points,omitempty"`
}

First one is ok, but I thought maybe there is a way to make 'Points' field in second one to be just a slice of Point, not slice of pointers?

There are two reasons for that:

  1. In case of slices Go runtime will guarantee that they are stored in one continuous memory. But in case of pointers they will be stored potentially in different memory regions, which will slow down operations.
  2. It's easier for golang's GC to collect garbage when it's a big chunk of that, not when there are a lot of pointers. This might be important in case you have multiple GB of such data.
awalterschulze commented 7 years ago

Maybe this will help?

import "github.com/gogo/protobuf/gogoproto/gogo.proto";

message Point {
  uint32 timestamp = 1;
  double value = 2;
}

message Metric {
  string metric = 1;
  repeated Point points = 2 [(gogoproto.nullable) = false];
}
Civil commented 7 years ago

Thanks! Missed that.

awalterschulze commented 7 years ago

My pleasure. May I ask where you are using gogoprotobuf?

Civil commented 7 years ago

Mainly here: https://github.com/go-graphite/carbonzipper/tree/master/carbonzipperpb3

I'm also writing graphite-compatible relay and ideally I want to send protobuf messages across the pipeline, but it's not on github yet.

awalterschulze commented 7 years ago

Cool could I add carbonzipper to the gogoprotobuf user list, or is it too early?

Civil commented 7 years ago

Well, it have some users at this moment. That's actually 4 pieces of software: go-graphite/carbonzipper, go-graphite/carbonapi, go-graphite/carbonsearch and lomik/go-carbon.

So I think yes, you can, we are using it for approx. 2 years.

awalterschulze commented 7 years ago

Really cool :) How about I add go-carbon and go-graphite. That seems like its the more official names?

Civil commented 7 years ago

That's ok :)

As about go-graphite it might still be better known as a carbonapi and carbonzipper, because for a really long time it was stored in separate repos.

As far as I know it's used by several companies, like eBay Classifieds group and Slack (there was a "Our many monsters" talk on Monitorama 2017 where they've mentioned that). So maybe it's better to mention that as "carbonzipper" stack with link to go-graphite org repos. But that's up to you.

awalterschulze commented 7 years ago

Cool I'll do that. On a side note I don't see carbonapi or carbonzipper or go-graphite being used here at eBay Classifieds Group. Maybe you can give me a contact on someone I can ask to confirm.

Civil commented 7 years ago

@favoretti (https://github.com/favoretti)

favoretti commented 7 years ago

All over the place in fact :) @deniszh

deniszh commented 7 years ago

Confirming that 👍

awalterschulze commented 7 years ago

My bad. Thats great news :)