Skip to content
New issue

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

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

Already on GitHub? # to your account

proposal: Allow the custom template for exec code generation #3371

Open
OldBigBuddha opened this issue Nov 12, 2024 · 2 comments
Open

proposal: Allow the custom template for exec code generation #3371

OldBigBuddha opened this issue Nov 12, 2024 · 2 comments

Comments

@OldBigBuddha
Copy link

What happened?

The following template generates an unlimited number of goroutines at once, which sometimes requires a large amount of memory.

go f(i)

For example, the minimum size of a goroutine is 2KiB, so processing 10,000 objects that have some fields with child resolvers would require more than 20 MiB.

What did you expect?

I hope that If I specify a template as follows, you want to use it for code generation in exec:

exec:
  filename: graph/generated.go
  package: generated
  exec_template: graph/template/exec.gotpl

backgroud

My real goal is to restrict the number of generating goroutine somehow, like using semaphore.

The following is a part of the generated.go file generated by the latest version of gqlgen:

ret := make(graphql.Array, len(v))
var wg sync.WaitGroup
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 wg.Done()
        }
        ret[i] = ec.marshalNTodo2ᚖgithubᚗcomᚋOldBigBuddhaᚋgqlgenᚑgoroutineᚑrestrictionᚑworkaroundᚋgraphᚋmodelᚐTodo(ctx, sel, v[i])
    }
    if isLen1 {
        f(i)
    } else {
        go f(i)
    }

}
wg.Wait()

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

return ret

If I can rewrite it like this, I can limit the number of goroutines created.

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ᚑgoroutineᚑrestrictionᚑworkaroundᚋ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

However, this change will affect many environments, mainly in terms of performance. Therefore, it would be great if the template used to generate this code could be made configurable like #2720.

I am already working on implementing this, and have confirmed that it is possible to use custom templates simply.

OldBigBuddha#1

I plan to submit a final version of this implementation as a PR, but I wanted to create an issue beforehand.

Minimal graphql.schema and models to reproduce

# GraphQL schema example
#
# https://gqlgen.com/getting-started/

type Todo {
  id: ID!
  text: String!
  done: Boolean!
  user: User!
}

type User {
  id: ID!
  name: String!
}

type Query {
  todos: [Todo!]!
}

input NewTodo {
  text: String!
  userId: String!
}

type Mutation {
  createTodo(input: NewTodo!): Todo!
}

versions

  • go run github.com/99designs/gqlgen version: v0.17.56
  • go version: 1.22.5
@OldBigBuddha OldBigBuddha changed the title Allow the custom template for exec code generation proposal: Allow the custom template for exec code generation Nov 14, 2024
@StevenACoffman
Copy link
Collaborator

@OldBigBuddha Thanks for your PR! I try to be responsive to monitoring PRs, as it proves people have dedicated time and energy to improving gqlgen for everyone's benefit. However, I cannot volunteer enough time to monitor and support issues without accompanying PRs.

I'm going to reply in your issue here, because your specific desired outcome (limit go routines) and your proposed solution (BYO server codegen template) are worthy of separate discussions.

There have been several previous attempts to either eliminate the use of goroutines here or to limit them. For example, #3203 attempted to add a @concurrent directive, but unintentionally broke the use of dataloaders, even though there was a follow-up #3286, we needed to get back to a working state that didn't break people's existing applications.

Concurrency is always tricky, and doing it inside of codegen templates makes it even harder to read and maintain.

If all you want is the ability to add a semaphore, then the simplest is to add a worker_limit integer config option that is surfaced to this template code so it could conditionally make your minor change in output:
Screenshot 2024-11-14 at 9 27 34 AM

This would allow people to control the degree of concurrency here and pick their own tradeoff between memory and speed of resolution.

However, your current PR allows people to customize the execution to a much greater extent, without advertising that your PR's config option is the mechanism to control the tradeoff between memory usage (because of the concurrency) and speed. Casual users (or less inexperienced Go developers) would have trouble figuring out how to do what you do.

@StevenACoffman
Copy link
Collaborator

I volunteer to maintain gqlgen, mostly by myself, with a few occasional contributors, so I'm always alert for things that might reduce people's desire to contribute back to gqlgen.

Maybe I'm overthinking it, but while your current PR is a good solution to a host of potential problems, I'm not sure that it is the best way to solve your particular problem, and I worry that it might have negative long term consequences.

Enabling people to greatly customize the execution could fracture the community, as experienced and well-resourced organizations would instead invest in their private execution templates without feeling much need to contribute back to gqlgen broadly useful improvements (like your ability to limit concurrency).

Currently, when a large organization like reddit/uber/dgraph/etc. privately forks gqlgen (or any open source software), those organizations no longer benefit from ecosystem contributions (like GraphQL spec changes) or have to painfully reconcile their own changes. It's just easier for those organization to contribute their internal improvements back upstream to the benefit of all. Your PR makes it easy to continue to enjoy all the benefits of other gqlgen community improvements without having to go to any effort to upstream their private execution improvements.

I'm not sure though, so what do you think?

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants