[
https://issues.apache.org/jira/browse/THRIFT-2451?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=13965771#comment-13965771
]
Aleksey Pesternikov commented on THRIFT-2451:
---------------------------------------------
>The test you wrote is a microbenchmark... i.e., of course it's faster. The big
>question (IMO) is whether it would actually show up in the >profile of a real
>production workload; of course I'm not demanding that you write such a test
>(super painful to do, of course), but this is >not how thrift is used in the
>wild:
Production workloads are different. Thats why YMMV, disclamer etc etc are in
the readme. All benchmarks in a world are more or less artificial. This one is
a part of my real use case, when log records are processed by M/R-like workers
and the structure is being created over and over again, just to be decoded from
the stream, checked for some condition and mostly discarded. It is also a part
of every thrift call, but impact is not that big of course.
Also, IMHO non-pointers are much more intuitive and easy to use. As I already
said, I would love to implement a (non-default) mapping mode when all optional
fields use zero value as IsSet indicator. It is very natural for golang. And
no, it is not broken as long as it is non-default and documented so people
understand what they are doing.
As a middle ground, I can return pointers for all optional fields as default
mapping, and implement this one, as well as proposed as nondefault. For the
record: I totally oppose hiding real struct fields behind getters/setters,
since it makes code so clunky.
[~jensg]?
> Do not use pointers for optional fields with defaults. Do not write such
> fields if its value set to default. Also, do not use pointers for any
> optional fields mapped to go map or slice. generate Get accessors
> ----------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------------
>
> Key: THRIFT-2451
> URL: https://issues.apache.org/jira/browse/THRIFT-2451
> Project: Thrift
> Issue Type: Bug
> Components: Go - Compiler
> Reporter: Aleksey Pesternikov
> Assignee: Jens Geyer
> Attachments:
> thrift-2451-getters-and-no-pointers-for-optional-with-default.patch
>
>
> Currently, for optional fields in struct
> {code}
> struct pkt {
> 1: optional string s = "DEFAULT",
> 2: optional i64 i = 42,
> 3: optional bool b = false
> }
> {code}
> go compiler generates the following:
> {code}
> type Pkt struct {
> S *string `thrift:"s,1"`
> I *int64 `thrift:"i,2"`
> B *bool `thrift:"b,3"`
> }
> func NewPkt() *Pkt {
> rval := &Pkt{
> S: new(string),
> I: new(int64),
> B: new(bool),
> }
> *(rval.S) = "DEFAULT"
> *(rval.I) = 42
> *(rval.B) = false
> return rval
> }
> func (p *Pkt) IsSetS() bool {
> return p.S != nil
> }
> func (p *Pkt) IsSetI() bool {
> return p.I != nil
> }
> func (p *Pkt) IsSetB() bool {
> return p.B != nil
> }
> {code}
> which is wrong in multiple ways:
> 1. Freshly initialized fields returns IsSetField() true
> http://play.golang.org/p/T2pIX80ZJp
> This results in
> a. wrong semantics: freshly created struct has optional fields set
> b. excessive payload produced on serialization (writing field value
> instead of skipping it)
> 2. Additional load on garbage collector
> 3. accessing field value is complicated and error prone. even without default
> value:
> {code}
> if pkt.IsSetB() && *pkt.B {
> //do something for b==true
> }
> {code}
> would work for false default for field b. However, if I change default
> value to true, I need to change all occurrences in the code like this:
> {code}
> if !pkt.IsSetB() || *pkt.B {
> //do something for b==true
> }
> {code}
> How to fix that?
> there are two ways:
> 1. get back to generating inlines instead of pointers for optional fields
> with default value and compare with "magic value" of default in IsSet*().
> could be tricky since not all types are comparable
> http://golang.org/ref/spec#Comparison_operators . notably, slices and maps
> are not.
> 2. approach, used in protobuf: Do not initialize optional fields, generate
> Get*() accessors like this:
> {code}
> var Pkt_B_Default = false
> func (p *Pkt) GetB() bool {
> if p.B == nil {
> return Pkt_B_Default
> }
> return *p.B
> }
> {code}
> Just to make API uniform, we can also generate accessors for required fields:
> {code}
> func (p *Pkt) GetB() bool {
> return p.B
> }
> {code}
> I'm inclining to implement second approach, but I would like to collect
> opinions before I dig into the code.
--
This message was sent by Atlassian JIRA
(v6.2#6252)
