Skip to content

Commit

Permalink
Optimize performance for binder, imports and packages (Rebased from s…
Browse files Browse the repository at this point in the history
…balabanov/master) (#1711)

* Cache go.mod resolution for module name search

* Optimize binder.FindObject() for performance by eliminating repeatitive constructs

* Optimize allocations in packages.Load() function

* Optimize binder.FindObject() by indexing object definitions for each loaded package

* goimports to fix linting

Signed-off-by: Steve Coffman <steve@khanacademy.org>

Co-authored-by: Sergey Balabanov <sergeyb@uber.com>
  • Loading branch information
StevenACoffman and sbalabanov-zz authored Nov 13, 2021
1 parent 237a7e6 commit 402a225
Show file tree
Hide file tree
Showing 53 changed files with 225 additions and 160 deletions.
5 changes: 1 addition & 4 deletions api/option_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -11,8 +11,7 @@ import (
"github.com/stretchr/testify/require"
)

type testPlugin struct {
}
type testPlugin struct{}

// Name returns the plugin name
func (t *testPlugin) Name() string {
Expand All @@ -25,7 +24,6 @@ func (t *testPlugin) MutateConfig(_ *config.Config) error {
}

func TestReplacePlugin(t *testing.T) {

t.Run("replace plugin if exists", func(t *testing.T) {
pg := []plugin.Plugin{
federation.New(),
Expand Down Expand Up @@ -54,5 +52,4 @@ func TestReplacePlugin(t *testing.T) {
require.EqualValues(t, resolvergen.New(), pg[1])
require.EqualValues(t, expectedPlugin, pg[2])
})

}
1 change: 0 additions & 1 deletion client/websocket.go
Original file line number Diff line number Diff line change
Expand Up @@ -64,7 +64,6 @@ func (p *Client) WebsocketWithPayload(query string, initPayload map[string]inter
srv := httptest.NewServer(p.h)
host := strings.ReplaceAll(srv.URL, "http://", "ws://")
c, _, err := websocket.DefaultDialer.Dial(host+r.URL.Path, r.Header)

if err != nil {
return errorSubscription(fmt.Errorf("dial: %w", err))
}
Expand Down
4 changes: 2 additions & 2 deletions cmd/init.go
Original file line number Diff line number Diff line change
Expand Up @@ -187,10 +187,10 @@ func fileExists(filename string) bool {
}

func initFile(filename, contents string) error {
if err := os.MkdirAll(filepath.Dir(filename), 0755); err != nil {
if err := os.MkdirAll(filepath.Dir(filename), 0o755); err != nil {
return fmt.Errorf("unable to create directory for file '%s': %w\n", filename, err)
}
if err := ioutil.WriteFile(filename, []byte(contents), 0644); err != nil {
if err := ioutil.WriteFile(filename, []byte(contents), 0o644); err != nil {
return fmt.Errorf("unable to write file '%s': %w\n", filename, err)
}

Expand Down
77 changes: 49 additions & 28 deletions codegen/config/binder.go
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,8 @@ import (
"go/token"
"go/types"

"golang.org/x/tools/go/packages"

"github.com/99designs/gqlgen/codegen/templates"
"github.com/99designs/gqlgen/internal/code"
"github.com/vektah/gqlparser/v2/ast"
Expand All @@ -15,11 +17,12 @@ var ErrTypeNotFound = errors.New("unable to find type")

// Binder connects graphql types to golang types using static analysis
type Binder struct {
pkgs *code.Packages
schema *ast.Schema
cfg *Config
References []*TypeReference
SawInvalid bool
pkgs *code.Packages
schema *ast.Schema
cfg *Config
References []*TypeReference
SawInvalid bool
objectCache map[string]map[string]types.Object
}

func (c *Config) NewBinder() *Binder {
Expand Down Expand Up @@ -78,8 +81,10 @@ func (b *Binder) FindType(pkgName string, typeName string) (types.Type, error) {
return obj.Type(), nil
}

var MapType = types.NewMap(types.Typ[types.String], types.NewInterfaceType(nil, nil).Complete())
var InterfaceType = types.NewInterfaceType(nil, nil)
var (
MapType = types.NewMap(types.Typ[types.String], types.NewInterfaceType(nil, nil).Complete())
InterfaceType = types.NewInterfaceType(nil, nil)
)

func (b *Binder) DefaultUserObject(name string) (types.Type, error) {
models := b.cfg.Models[name].Model
Expand Down Expand Up @@ -112,45 +117,61 @@ func (b *Binder) FindObject(pkgName string, typeName string) (types.Object, erro
if pkgName == "" {
return nil, fmt.Errorf("package cannot be nil")
}
fullName := typeName
if pkgName != "" {
fullName = pkgName + "." + typeName
}

pkg := b.pkgs.LoadWithTypes(pkgName)
if pkg == nil {
err := b.pkgs.Errors()
if err != nil {
return nil, fmt.Errorf("package could not be loaded: %s: %w", fullName, err)
return nil, fmt.Errorf("package could not be loaded: %s.%s: %w", pkgName, typeName, err)
}
return nil, fmt.Errorf("required package was not loaded: %s", fullName)
return nil, fmt.Errorf("required package was not loaded: %s.%s", pkgName, typeName)
}

if b.objectCache == nil {
b.objectCache = make(map[string]map[string]types.Object, b.pkgs.Count())
}

defsIndex, ok := b.objectCache[pkgName]
if !ok {
defsIndex = indexDefs(pkg)
b.objectCache[pkgName] = defsIndex
}

// function based marshalers take precedence
for astNode, def := range pkg.TypesInfo.Defs {
// only look at defs in the top scope
if def == nil || def.Parent() == nil || def.Parent() != pkg.Types.Scope() {
continue
}
if val, ok := defsIndex["Marshal"+typeName]; ok {
return val, nil
}

if astNode.Name == "Marshal"+typeName {
return def, nil
}
if val, ok := defsIndex[typeName]; ok {
return val, nil
}

// then look for types directly
return nil, fmt.Errorf("%w: %s.%s", ErrTypeNotFound, pkgName, typeName)
}

func indexDefs(pkg *packages.Package) map[string]types.Object {
res := make(map[string]types.Object)

scope := pkg.Types.Scope()
for astNode, def := range pkg.TypesInfo.Defs {
// only look at defs in the top scope
if def == nil || def.Parent() == nil || def.Parent() != pkg.Types.Scope() {
if def == nil {
continue
}
parent := def.Parent()
if parent == nil || parent != scope {
continue
}

if astNode.Name == typeName {
return def, nil
if _, ok := res[astNode.Name]; !ok {
// The above check may not be really needed, it is only here to have a consistent behavior with
// previous implementation of FindObject() function which only honored the first inclusion of a def.
// If this is still needed, we can consider something like sync.Map.LoadOrStore() to avoid two lookups.
res[astNode.Name] = def
}
}

return nil, fmt.Errorf("%w: %s", ErrTypeNotFound, fullName)
return res
}

func (b *Binder) PointerTo(ref *TypeReference) *TypeReference {
Expand Down Expand Up @@ -236,12 +257,12 @@ func (t *TypeReference) IsScalar() bool {
}

func (t *TypeReference) UniquenessKey() string {
var nullability = "O"
nullability := "O"
if t.GQL.NonNull {
nullability = "N"
}

var elemNullability = ""
elemNullability := ""
if t.GQL.Elem != nil && t.GQL.Elem.NonNull {
// Fix for #896
elemNullability = "ᚄ"
Expand Down
1 change: 0 additions & 1 deletion codegen/config/config_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -128,7 +128,6 @@ func TestReferencedPackages(t *testing.T) {

assert.Equal(t, []string{"github.com/test", "github.com/otherpkg"}, pkgs)
})

}

func TestConfigCheck(t *testing.T) {
Expand Down
2 changes: 0 additions & 2 deletions codegen/templates/import_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -54,7 +54,6 @@ func TestImports(t *testing.T) {
require.Equal(t, fmt.Sprintf("bar%d", i), a.Lookup(cBar))
} else {
require.Equal(t, "bar", a.Lookup(cBar))

}
}
})
Expand Down Expand Up @@ -89,5 +88,4 @@ turtles "github.com/99designs/gqlgen/codegen/templates/testdata/pkg_mismatch"`,
require.Equal(t, `abar "github.com/99designs/gqlgen/codegen/templates/testdata/a/bar"
bbar "github.com/99designs/gqlgen/codegen/templates/testdata/b/bar"`, a.String())
})

}
4 changes: 2 additions & 2 deletions codegen/templates/templates.go
Original file line number Diff line number Diff line change
Expand Up @@ -589,7 +589,7 @@ func render(filename string, tpldata interface{}) (*bytes.Buffer, error) {
}

func write(filename string, b []byte, packages *code.Packages) error {
err := os.MkdirAll(filepath.Dir(filename), 0755)
err := os.MkdirAll(filepath.Dir(filename), 0o755)
if err != nil {
return fmt.Errorf("failed to create directory: %w", err)
}
Expand All @@ -600,7 +600,7 @@ func write(filename string, b []byte, packages *code.Packages) error {
formatted = b
}

err = ioutil.WriteFile(filename, formatted, 0644)
err = ioutil.WriteFile(filename, formatted, 0o644)
if err != nil {
return fmt.Errorf("failed to write %s: %w", filename, err)
}
Expand Down
1 change: 0 additions & 1 deletion codegen/templates/templates_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -70,7 +70,6 @@ func TestToGoPrivate(t *testing.T) {
}

func Test_wordWalker(t *testing.T) {

helper := func(str string) []*wordInfo {
resultList := []*wordInfo{}
wordWalker(str, func(info *wordInfo) {
Expand Down
1 change: 0 additions & 1 deletion codegen/testserver/followschema/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,4 @@ func TestMiddleware(t *testing.T) {

require.NoError(t, err)
require.True(t, called)

}
6 changes: 2 additions & 4 deletions codegen/testserver/followschema/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ type ForcedResolver struct {
Field Circle
}

type ModelMethods struct {
}
type ModelMethods struct{}

func (m ModelMethods) NoContext() bool {
return true
Expand Down Expand Up @@ -58,8 +57,7 @@ func (m MarshalPanic) MarshalGQL(w io.Writer) {
panic("BOOM")
}

type Panics struct {
}
type Panics struct{}

func (p *Panics) FieldFuncMarshal(ctx context.Context, u []MarshalPanic) []MarshalPanic {
return []MarshalPanic{MarshalPanic("aa"), MarshalPanic("bb")}
Expand Down
1 change: 0 additions & 1 deletion codegen/testserver/followschema/ptr_to_slice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,5 @@ func TestPtrToSlice(t *testing.T) {
require.NoError(t, err)

require.Equal(t, []string{"hello"}, resp.PtrToSliceContainer.PtrToSlice)

})
}
7 changes: 5 additions & 2 deletions codegen/testserver/followschema/scalar_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,16 @@ type StringFromContextInterface struct {
OperationName string
}

var _ graphql.ContextMarshaler = StringFromContextInterface{}
var _ graphql.ContextUnmarshaler = (*StringFromContextInterface)(nil)
var (
_ graphql.ContextMarshaler = StringFromContextInterface{}
_ graphql.ContextUnmarshaler = (*StringFromContextInterface)(nil)
)

func (StringFromContextInterface) MarshalGQLContext(ctx context.Context, w io.Writer) error {
io.WriteString(w, strconv.Quote(graphql.GetFieldContext(ctx).Field.Name))
return nil
}

func (i *StringFromContextInterface) UnmarshalGQLContext(ctx context.Context, v interface{}) error {
i.OperationName = graphql.GetFieldContext(ctx).Field.Name
return nil
Expand Down
1 change: 0 additions & 1 deletion codegen/testserver/followschema/scalar_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ func TestFloatInfAndNaN(t *testing.T) {
err := c.Post(`query { infinity }`, nil)
require.Error(t, err)
})

}

func TestContextPassedToMarshal(t *testing.T) {
Expand Down
6 changes: 2 additions & 4 deletions codegen/testserver/followschema/v-ok.go
Original file line number Diff line number Diff line change
@@ -1,16 +1,14 @@
package followschema

// VOkCaseValue model
type VOkCaseValue struct {
}
type VOkCaseValue struct{}

func (v VOkCaseValue) Value() (string, bool) {
return "hi", true
}

// VOkCaseNil model
type VOkCaseNil struct {
}
type VOkCaseNil struct{}

func (v VOkCaseNil) Value() (string, bool) {
return "", false
Expand Down
10 changes: 6 additions & 4 deletions codegen/testserver/followschema/wrapped_type.go
Original file line number Diff line number Diff line change
Expand Up @@ -2,7 +2,9 @@ package followschema

import "github.com/99designs/gqlgen/codegen/testserver/followschema/otherpkg"

type WrappedScalar = otherpkg.Scalar
type WrappedStruct otherpkg.Struct
type WrappedMap otherpkg.Map
type WrappedSlice otherpkg.Slice
type (
WrappedScalar = otherpkg.Scalar
WrappedStruct otherpkg.Struct
WrappedMap otherpkg.Map
WrappedSlice otherpkg.Slice
)
1 change: 0 additions & 1 deletion codegen/testserver/singlefile/middleware_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -106,5 +106,4 @@ func TestMiddleware(t *testing.T) {

require.NoError(t, err)
require.True(t, called)

}
6 changes: 2 additions & 4 deletions codegen/testserver/singlefile/models.go
Original file line number Diff line number Diff line change
Expand Up @@ -10,8 +10,7 @@ type ForcedResolver struct {
Field Circle
}

type ModelMethods struct {
}
type ModelMethods struct{}

func (m ModelMethods) NoContext() bool {
return true
Expand Down Expand Up @@ -58,8 +57,7 @@ func (m MarshalPanic) MarshalGQL(w io.Writer) {
panic("BOOM")
}

type Panics struct {
}
type Panics struct{}

func (p *Panics) FieldFuncMarshal(ctx context.Context, u []MarshalPanic) []MarshalPanic {
return []MarshalPanic{MarshalPanic("aa"), MarshalPanic("bb")}
Expand Down
1 change: 0 additions & 1 deletion codegen/testserver/singlefile/ptr_to_slice_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,5 @@ func TestPtrToSlice(t *testing.T) {
require.NoError(t, err)

require.Equal(t, []string{"hello"}, resp.PtrToSliceContainer.PtrToSlice)

})
}
7 changes: 5 additions & 2 deletions codegen/testserver/singlefile/scalar_context.go
Original file line number Diff line number Diff line change
Expand Up @@ -12,13 +12,16 @@ type StringFromContextInterface struct {
OperationName string
}

var _ graphql.ContextMarshaler = StringFromContextInterface{}
var _ graphql.ContextUnmarshaler = (*StringFromContextInterface)(nil)
var (
_ graphql.ContextMarshaler = StringFromContextInterface{}
_ graphql.ContextUnmarshaler = (*StringFromContextInterface)(nil)
)

func (StringFromContextInterface) MarshalGQLContext(ctx context.Context, w io.Writer) error {
io.WriteString(w, strconv.Quote(graphql.GetFieldContext(ctx).Field.Name))
return nil
}

func (i *StringFromContextInterface) UnmarshalGQLContext(ctx context.Context, v interface{}) error {
i.OperationName = graphql.GetFieldContext(ctx).Field.Name
return nil
Expand Down
1 change: 0 additions & 1 deletion codegen/testserver/singlefile/scalar_context_test.go
Original file line number Diff line number Diff line change
Expand Up @@ -23,7 +23,6 @@ func TestFloatInfAndNaN(t *testing.T) {
err := c.Post(`query { infinity }`, nil)
require.Error(t, err)
})

}

func TestContextPassedToMarshal(t *testing.T) {
Expand Down
Loading

0 comments on commit 402a225

Please sign in to comment.