vektah / dataloaden

go generate based DataLoader
MIT License
530 stars 79 forks source link

Replace code generation with generics #61

Open lwc opened 2 years ago

lwc commented 2 years ago

Mostly done for my own edification.

Intentionally kept the API as similar as possible to demonstrate the existing test cases still pass (with one exception, noted).

Should keep the migration path very straightforward from generated to generic.

n33pm commented 2 years ago

code looks good to me...

n33pm commented 2 years ago

code looks good to me...

  • added test on your pr. we dont need tests on the example anymore.
  • README.md needs updates too.
  • version update because its a breaking change

https://github.com/numero33/dataloaden

frederikhors commented 2 years ago

Hi guys, I would like to bring to your attention the following project: https://github.com/vikstrous/dataloadgen which has great benchmarks!

What do you think?

n33pm commented 2 years ago

Hi guys, I would like to bring to your attention the following project: https://github.com/vikstrous/dataloadgen which has great benchmarks!

What do you think?

benchmarks are similar and on my hand i use concurrently far more often than cached values. don't get me wrong but I don't think it's optimal on vikstrous/dataloadgen if you cache the whole function.

dl := dataloaden.NewLoader(dataloaden.LoaderConfig[int, benchmarkUser]{
        Wait:     500 * time.Nanosecond,
        MaxBatch: 100,
        Fetch: func(keys []int) ([]benchmarkUser, []error) {
            users := make([]benchmarkUser, len(keys))
            errors := make([]error, len(keys))

            for i, key := range keys {
                if key%100 == 1 {
                    errors[i] = fmt.Errorf("user not found")
                } else if key%100 == 1 {
                    users[i] = benchmarkUser{}
                } else {
                    users[i] = benchmarkUser{ID: strconv.Itoa(key), Name: "user " + strconv.Itoa(key)}
                }
            }
            return users, errors
        },
    })
cpu: Intel(R) Core(TM) i7-6920HQ CPU @ 2.90GHz
BenchmarkDataloadgen
BenchmarkDataloadgen/caches
BenchmarkDataloadgen/caches-8           17150414            65.49 ns/op
BenchmarkDataloadgen/random_spread
BenchmarkDataloadgen/random_spread-8     2686813           649.7 ns/op
BenchmarkDataloadgen/concurently
BenchmarkDataloadgen/concurently-8         89816         14618 ns/op
BenchmarkN33Dataloaden
BenchmarkN33Dataloaden/caches
BenchmarkN33Dataloaden/caches-8         12214902            92.20 ns/op
BenchmarkN33Dataloaden/random_spread
BenchmarkN33Dataloaden/random_spread-8           2403955           532.4 ns/op
BenchmarkN33Dataloaden/concurently
BenchmarkN33Dataloaden/concurently-8               82574         13577 ns/op
frederikhors commented 2 years ago

@StevenACoffman can you operate on this repo?

Instead of a new version on this package, can we add this to gqlgen? Like gqlgen/dataloaden or gqlgen/dataloadgen?

We could also write in Readme that the project comes from this repo and new credits to @lwc.

StevenACoffman commented 2 years ago

I do have access to merge pull requests in this repository, and I don't mind merging this PR if @lwc cannot.

I don't think that this repo should be folded into gqlgen though.

I would rather than the community coalesced and collaborated on a single dataloader implementation, I don't think we are quite there yet.

Currently graph-gophers/graphql-go and 99designs/gqlgen can pick from any of the three mentioned dataloaders:

At Khan Academy, we currently use (and prefer) graph-gophers/dataloader so I would personally rather not add vektah/dataloaden to the gqlgen repository. Also, we are stuck on Go 1.16 until GCP AppEngine supports a newer version, so Khan can't use generics yet.

For those reasons, I don't think I can allocate time to maintain this repository until Khan Academy was actively using it.

frederikhors commented 2 years ago

@lwc what do you think about creating a new repo since it’s all new?

Warashi commented 2 years ago

hello, I created a repository Warashi/dataloaden. which contains generics version of vektah/dataloaden. If you want to use generics version of dataloaden, please consider to use Warashi/dataloaden.

StevenACoffman commented 2 years ago

@Warashi Have you compared your project to https://github.com/vikstrous/dataloadgen ? I wonder if you could collaborate.

Warashi commented 2 years ago

@StevenACoffman oh, I had missed dataloadgen. I would love to collaborate with you on this.