DataLoaderRegistry inefficiency

[benmccann]

The DataLoaderRegistry should be request-scoped. That means that users are creating every DataLoader for every request even if none or only one or two are used.

Right now we would do something like:

    dataLoaderRegistry.register("user", DataLoader.newDataLoader(userBatchLoader));

We could probably fix this by making DataLoaderReigstry a singleton and instead registering a function that would provide a DataLoader:

    dataLoaderRegistry.register("user", () -> DataLoader.newDataLoader(userBatchLoaderProvider.get()));

[bbakerman]

This makes a lot of sense. DataLoaderRegistry is in the https://github.com/graphql-java/java-dataloader code base.

I have raised this issue there as well

graphql-java/java-dataloader#67

[bbakerman]

I looked into this and it's really kinda pointless.

We can make it lazy but as soon as you called dispatchAll the it needs to materialise the dataloaders and dispatch them

We would have to write a fancy MemoizingSupplierWithTracking that could decide if its every been asked for and then filter them out from the list. In short I am not sure that its worth it.

I would love to see more evidence now of the costs of creating a DL even if its not needed in a certain graphql request.

[mumutu66]

I also found it is a problem in real world when the schema may contains thousand business objects and every toOne related query will cause an "n +1" problem and dataloaders is required , if the server has no sense a query will hit how many toOne query , then it should create thousand dataloader which will definitely slow down the server. so one way is to do the lazy loader as @benmccann mentioned and do the left optimization or if there has certain tool can know which dataloader is needed from every query string

[bbakerman]

org.dataloader.DataLoaderRegistry#computeIfAbsent now exists - you can lazily create them.

Since DataLoaders are stateful, they themselves must be per request otherwise badness will happen.