99designs / 99designs/gqlgen

NewScalarFieldContext's fast path allocates an error on every field resolution, unconditionally

Đang mở
#4,294 1 bình luận 0 reaction 0 người được giao Xem trên GitHub
Ngôn ngữ chính
Go
Star
10.8k
Fork
1.3k
Merge trung bình
2 ngày 36 phút
Pull request đã merge (30 ngày)
26

Mô tả

### What

`graphql.NewScalarFieldContext`'s fast path (added in #4086 for argument-less
scalar/enum fields) is called from every generated `fieldContext_*` function
for such a field, and each call builds a fresh `errors.New(...)`:

```go
return graphql.NewScalarFieldContext(objectName, field, isMethod, isResolver,
errors.New("field of type String does not have child fields"))
```

That error is only ever read if a query illegally selects sub-fields on a
scalar — essentially never. It's still built on every call, because it's a
function argument and Go evaluates it eagerly regardless of whether the
callee uses it.

### Measured cost

Benchmarked the generated `fieldContext_Todo_id` in `_examples/todo`,
same benchmark before/after a fix, results forced into a package-level sink
so the compiler can't optimize the call away:

```
before: 48.36 ns/op 184 B/op 3 allocs/op
after: 40.69 ns/op 168 B/op 2 allocs/op
```

One allocation removed — nothing else in the call chain changed. This is
per field, per resolution, unconditionally: a schema with 300 scalar/enum
leaf fields pays 300 avoidable allocations per operation that touches all of
them, regardless of query shape or whether anything actually errors.

### Is this a regression?

Yes, precisely traceable to #4086. Before that PR, every `fieldContext_*`
function — scalar or not, with args or without — used a single code path
that built this same error *inside* the `Child` closure, i.e. lazily,
evaluated only if `Child` is actually invoked. #4086 split that into a fast
path (argument-less scalar/enum fields) and a verbose fallback (fields with
args), to cut ~8 lines of generated code per field. The fallback kept the
lazy pattern; the fast path — the common case — didn't, because the error
became a plain eagerly-evaluated argument instead of closure-internal code.
#4086's own description claims "zero runtime performance impact," measured
in generated-line-count terms; this doesn't hold for allocations.

### Proposed fix

Since the message depends only on the GraphQL type name, and there's a
small, fixed set of those per schema, generate one shared error value per
distinct scalar/enum type name (mirroring the existing
`UniqueChildFieldTypes` / shared `childFields_*` pattern from #4086 itself),
and have the fast-path template reference it instead of calling `errors.New`
per field. `NewScalarFieldContext`'s public signature is unchanged — this
only changes what gets passed into it, so it's non-breaking.

Implemented, tested (`go test ./codegen/... ./graphql/... ./plugin/...`,
`golangci-lint`), and regenerated (root module + `_examples`) in draft PR
#4293 — filing this proposal per CONTRIBUTING.md before asking for review,
since it touches generated output for every scalar/enum field in every
schema. Open to feedback on approach (e.g. where the shared vars should live)
before marking it ready.

Hướng dẫn đóng góp

Mở hướng dẫn đóng góp

Đánh giá

Issue này chưa được đánh giá.

Nhận issue mới trong hộp thư của bạn

Bản tóm tắt ngắn những issue GitHub phù hợp với người mới.