THRIFT-5337 Go set fields write improvement

[BytedanceRPC]

There is a duplicate elements check for set in writeFields* function, and it compares elements using reflect.DeepEqual which is expensive.

It's much faster that generates a "DeepEqual" function for set elements and apply it in duplicate elements check, especially for nested struct element.

• Did you create an Apache Jira ticket? (not required for trivial changes)
• If a ticket exists: Does your pull request title follow the pattern "THRIFT-NNNN: describe my issue"?
• Did you squash your changes to a single commit? (not required, but preferred)
• Did you do your best to avoid breaking changes? If one was needed, did you label the Jira ticket with "Breaking-Change"?
• If your change does not involve any code, include [skip ci] anywhere in the commit message to free up build resources.

[fishy]

I haven't gone through all the changes yet, but wanted to give you some early feedback.

[fishy]

Can we rename all *deepequal* into *equals*? I don't think it's accurate to call them "deepequal".

[BytedanceRPC]

Thanks for review, equals sounds better.

[fishy]

this is unused and it will cause certain compilers to fail:

src/thrift/generate/t_go_generator.cc:1879:11: error: unused variable ‘field_id’ [-Werror=unused-variable]
 1879 |   int32_t field_id = -1;
      |           ^~~~~~~~
cc1plus: all warnings being treated as errors

[BytedanceRPC]

removed latest commit

[fishy]

1. wrong indentation
2. This generates code like this for optional fields:

if (*p.FieldN) != (*other.FieldN) { return false }

this will cause panic when the field is not set.

[BytedanceRPC]

Thanks, I add a pointer check to avoid panic last commit

[dcelasun]

Looks reasonable at a first glance, will do a proper review later.

I wonder if there are any cases covered by reflect.DeepEqual that aren't covered by this method. If there are, that would be a BC break and we'd have to mention it in the changelog.

[BytedanceRPC]

@dcelasun The process of Equals is pretty like Write (serialize). Since write can cover all cases, I believe so can Equals.

[fishy]

I like where this is going, but left some small comments.

Also I believe with this change there's no other way the reflect import is used, so we can totally remove it from compiled code now.

[fishy]

I believe currently there's some bug in your implementation. The result is different from reflect.DeepEqual, Here's the code I used:

test.thrift:

namespace go test

enum Enum {
  One = 1,
  Two = 2,
}

struct Struct {
  1: optional string Str,
  2: optional i32 I32,
  3: optional set<Enum> Enums,
}

bench_test.go:

package thriftsetbench

import (
        "reflect"
        "testing"

        "github.com/apache/thrift/lib/go/thrift"
        "github.com/fishy/thriftsetbench/gen-go/test"
)

func BenchmarkEqual(b *testing.B) {
        equalGetter := func() (*test.Struct, *test.Struct) {
                s1 := &test.Struct{
                        Str: thrift.StringPtr("foobar"),
                        Enums: []test.Enum{
                                test.Enum_One,
                                test.Enum_Two,
                        },
                }
                s2 := &test.Struct{
                        Str: thrift.StringPtr("foobar"),
                        Enums: []test.Enum{
                                test.Enum_One,
                                test.Enum_Two,
                        },
                }
                return s1, s2
        }
        nonEqualGetter := func() (*test.Struct, *test.Struct) {
                s1 := &test.Struct{
                        Str: thrift.StringPtr("foobar"),
                        Enums: []test.Enum{
                                test.Enum_One,
                        },
                }
                s2 := &test.Struct{
                        Str: thrift.StringPtr("foobar"),
                        Enums: []test.Enum{
                                test.Enum_Two,
                        },
                }
                return s1, s2
        }

        for label, getter := range map[string]func() (*test.Struct, *test.Struct){
                "equal":    equalGetter,
                "nonequal": nonEqualGetter,
        } {
                b.Run(label, func(b *testing.B) {
                        s1, s2 := getter()
                        reflectResult := reflect.DeepEqual(s1, s2)
                        equalsResult := s1.Equals(s2)
                        if reflectResult != equalsResult {
                                b.Errorf("Reflect result: %v, Equals result: %v", reflectResult, equalsResult)
                        }

                        b.Run("reflect", func(b *testing.B) {
                                b.RunParallel(func(pb *testing.PB) {
                                        for pb.Next() {
                                                reflect.DeepEqual(s1, s2)
                                        }
                                })
                        })

                        b.Run("equals", func(b *testing.B) {
                                b.RunParallel(func(pb *testing.PB) {
                                        for pb.Next() {
                                                s1.Equals(s2)
                                        }
                                })
                        })
                })
        }
}

result:

$ go test -bench . -benchmem
goos: linux
goarch: amd64
pkg: github.com/fishy/thriftsetbench
BenchmarkEqual/equal/reflect-12         10597189               113 ns/op              64 B/op          6 allocs/op
BenchmarkEqual/equal/equals-12          1000000000               0.680 ns/op           0 B/op          0 allocs/op
BenchmarkEqual/nonequal/reflect-12              12201334                99.9 ns/op            48 B/op          4 allocs/op
BenchmarkEqual/nonequal/equals-12               1000000000               0.712 ns/op           0 B/op          0 allocs/op
--- FAIL: BenchmarkEqual/nonequal
    bench_test.go:54: Reflect result: false, Equals result: true
--- FAIL: BenchmarkEqual
FAIL
exit status 1
FAIL    github.com/fishy/thriftsetbench 4.183s

We likely would need to have some thorough test for the Equals implementation with this PR.

[BytedanceRPC]

@fishy It's a bug in generate_go_equals_guard, I fixed it.
Yes, some thorough test is pretty necessary. I will also do more test.

[fishy]

@BytedanceRPC there's another go compiler change merged, so can you rebase your PR on latest master branch?

Also please add some tests for the Equals implementation. This change looks promising, but we need the confidence that it's implemented correctly.

[BytedanceRPC]

@fishy rebased PR on latest master branch, and added Equals test under lib/go/test

[fishy]

Overall lgtm, but there are a few things can be improved in the test code.

Also you can remove

thrift/compiler/cpp/src/thrift/generate/t_go_generator.cc

Line 927 in c843abb

system_packages.push_back("reflect");

and

thrift/compiler/cpp/src/thrift/generate/t_go_generator.cc

Line 952 in c843abb

"var _ = reflect.DeepEqual\n"

. Since this change eliminated the usage of reflect.DeepEqual, the reflect package is not used any more in compiled go code.

[fishy]

nit: I think we should do this check on the call site instead, and currently there's only one caller of it (Line 1497), so there should be:

if !is_result && !is_args {
  generate_go_struct_equals(out, tstruct, tstruct_name);
}

Semantically this function is called "generate_*", but when one of its arg is passed in true it doesn't generate anything, which feels wrong to me.

[BytedanceRPC]

looks reasonable, thanks for suggestion.

[fishy]

nit: this indentation looks weird. usually you either align with (, or use fixed 2/4/6 spaces. this is neither.

I don't believe clang-format is used to generate this indentation either, at least it's not enforced by travis. when I run clang-format -i --style=file compiler/cpp/src/thrift/generate/t_go_generator.cc it made a ton of changes in this file.

[BytedanceRPC]

not sure is the std:: making indentation looks weird. Indentations looks proper after I remove std:: and apply clang-format.
To avoid unnecessary change, I only apply the output of clang-format on my changes in compiler/cpp/src/thrift/generate/t_go_generator.cc.

[fishy]

These test failures should be t.Error instead of t.Fatal. Fatal stops the test from running, so if someone made a mistake in the future and failed multiple cases inside this test, they will only see the first failure in the output.

Fatal should only be used when the following tests are depending on this one (e.g. used for nil checks).

[BytedanceRPC]

Thanks for suggestion

[fishy]

what does this line do?

[BytedanceRPC]

try to keep generate_go_equals* with the same arguments, but ttype not really used here.

[fishy]

Thanks. Looks good. I'll merge it once the tests pass.

[fishy]

Can you squash your PR to a single commit, with proper commit message (what you used in the PR title and description)?

[fishy]

Travis should finish in 20min or so and I'll merge it after that.