Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

add worker_limit option for server code generation #3376

Merged
merged 6 commits into from
Nov 18, 2024

Conversation

OldBigBuddha
Copy link
Contributor

Overview

When executing a child resolver, a goroutine is created each time if there are multiple fields.

For example, if two fields are in 10,000 objects, and they are required to execute a child resolver, 20,000 goroutines will be created at once with a single request. Since each goroutine requires at least 2 KiB, each request requires approximately 41 MiB for ONLY goroutines.

In this PR, as discussed in #3371, I introduced an option to prevent unintended big memory pressure by restricting goroutines generated at once using a semaphore. To minimize the difference in automatically generated code between before and after this patch is merged, if worker_limit is not set, there is no diff from the previously generated code.

exec:
  filename: graph/generated.go
  package: graph
  worker_limit: 1000

If you set it as above, the following code will be generated.

ret := make(graphql.Array, len(v))
var wg sync.WaitGroup
sm := semaphore.NewWeighted(1000)
isLen1 := len(v) == 1
if !isLen1 {
wg.Add(len(v))
}
for i := range v {
i := i
fc := &graphql.FieldContext{
	Index:  &i,
	Result: &v[i],
}
ctx := graphql.WithFieldContext(ctx, fc)
f := func(i int) {
	defer func() {
		if r := recover(); r != nil {
			ec.Error(ctx, ec.Recover(ctx, r))
			ret = nil
		}
	}()
	if !isLen1 {
		defer func() {
			sm.Release(1)
			wg.Done()
		}()
	}
	ret[i] = ec.marshalNTodo2ᚖgithubᚗcomᚋOldBigBuddhaᚋgqlgenᚑplaygroundᚋgraphᚋmodelᚐTodo(ctx, sel, v[i])
}
if isLen1 {
	f(i)
} else {
	if err := sm.Acquire(ctx, 1); err != nil {
		ec.Error(ctx, ctx.Err())
	} else {
		go f(i)
	}
}

}
wg.Wait()

for _, e := range ret {
if e == graphql.Null {
	return graphql.Null
}
}

return ret

Note that worker_limit is an option that reduces memory consumption at the expense of concurrency, so excessive restrictions will unnecessarily prolong execution times and degrade application performance.

I appreciate @StevenACoffman for sharing this idea with me.

I have:

  • Added tests covering the bug / feature (see testing)
  • Updated any relevant documentation (see docs)

@OldBigBuddha OldBigBuddha marked this pull request as ready for review November 18, 2024 07:03
@coveralls
Copy link

Coverage Status

coverage: 59.578%. remained the same
when pulling 35e6a64 on OldBigBuddha:worker_limit
into 288848a on 99designs:master.

@StevenACoffman StevenACoffman merged commit 89c4e84 into 99designs:master Nov 18, 2024
16 of 17 checks passed
@OldBigBuddha OldBigBuddha deleted the worker_limit branch November 19, 2024 00:15
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants