WIP: Async and deferred loaders #173

tgwizard · 2024-10-06T11:27:27Z

Very WIP.

This PR introduces two new concepts to loaders:

A loader callback perform_on_wait that triggers whenever any batch loader promise starts waiting. This allows for async work to be triggered early in the lazy processing step (but still after the eager processing step has completed).
A loader method defer, which tells the executor to resolve other non-deferred loaders before the current one, to allow for non-async / non-concurrent loaders to run in a blocking manner while async loaders can wait to block for their result until the last minute.

See the examples/http_loader.rb file for how you'd do this. (I haven't tested that it works. Names not final).

This is inspired by https://graphql-ruby.org/dataloader/parallelism, which is a simpler solution, but based on fibers. Fibers are neat, but comes with other issues (e.g. around thread/fiber local state).

Before this PR, batch loaders could do concurrent operations within themselves. With this PR, batch loaders can do concurrent operations across all batch loaders.

(Actually, the http_loader example isn't great, since it's async/concurrent but not batching. It could start the futures in load, which is in the eager phase of resolution. This PR allows for batch loaders that are both async/concurrent and batching, and also concurrent with each other).

swalkinshaw · 2024-10-07T13:50:35Z

lib/graphql/batch/executor.rb

@@ -53,6 +53,19 @@ def resolve(loader)
      @loading = was_loading
    end

+    def defer(_loader)


This isn't used; is it needed at all?

Yes, probably not needed, I added it to make it similar to resolve.

swalkinshaw · 2024-10-07T14:02:19Z

lib/graphql/batch/loader.rb

@@ -126,6 +136,36 @@ def fulfilled?(key)
      promise.pending? && promise.source != self
    end

+    def perform_on_wait(keys)


If a loader opts into using perform_on_wait, should defer then always be called in perform? Or is there a situation where defer wouldn't be needed?

Hmm, good question.

I can't think of any such situation. Given that the perform_on_wait hook is always called first, all those async loaders will have their opportunity to run first, and then they can always defer and run at the very end. Perhaps we could simplify it so that the loader doesn't have to call defer, that's done automatically. Perhaps a new module that a loader can include like AsyncAndDeferred, which adds the on_any_wait hook etc.

Not entirely sure that's better.

I like that better, especially for the reason that it avoids a potential footgun of not calling defer manually. @amomchilov also mentioned expressing this as a separate class, so maybe the module is the solution.

I pushed a commit with a GraphQL::Batch::Async module. What do you think @amomchilov @swalkinshaw ?

amomchilov

General idea

I really like this feature idea, and it's definitely addressing a real need that've we've repeatedly encountered.

Having the parallelism limited to "within" the batch loader has been a real issue, that unnecessarily holds back the execution of the promises for already-done work, until all work is done.

Documentation considerations

These are really tricky things, that I believe would be difficult for most devs to approach without significant prior experience in this area.

E.g. this is a really nuanced distinction, that I only understand because I've been in these particular weeds before :D

Actually, the http_loader example isn't great, since it's async/concurrent but not batching. It could start the futures in load, which is in the eager phase of resolution. This PR allows for batch loaders that are both async/concurrent and batching, and also concurrent with each other

I think adding more method-level docs on the relevant pieces would help, but also a more complete "guide" style docs, like on the README

Implementation considerations

I wonder if these different behaviours could be better expressed if they were two separate types? Naming is hard, but perhaps something like StreamableLoader vs BatchLoader?

Would love to hear your thoughts! Thanks for working on this

amomchilov · 2024-10-07T15:25:08Z

examples/http_loader.rb

      # At this point, all of the concurrent work has been started.

      # This converges back in, waiting on each concurrent future to finish, and fulfilling each
      # (non-concurrent) Promise.rb promise.
      operations.each_with_index.each do |operation, index|
-        fulfill(operation, futures[index].value) # .value is a blocking call
+        fulfill(operation, futures[index].value!) # .value is a blocking call


Suggested change

fulfill(operation, futures[index].value!) # .value is a blocking call

fulfill(operation, futures[index].value!) # .value! is a blocking call

amomchilov · 2024-10-07T15:27:03Z

examples/http_loader.rb

    end

-    def perform(operations)
+    def perform_on_wait(operations)


Should this perhaps be:

Suggested change

def perform_on_wait(operations)

def perform_on_any_wait(operations)

To match the naming of Loader#on_any_wait

amomchilov · 2024-10-07T15:28:51Z

lib/graphql/batch/loader.rb

@@ -126,6 +136,36 @@ def fulfilled?(key)
      promise.pending? && promise.source != self
    end

+    def perform_on_wait(keys)
+      # FIXME: Better name?
+      # Interface to add custom code to e.g. trigger async operations when any loader starts waiting.


Could you move these comments into the method docs above the def? That way they show on hover, in generated docs, etc.

gmac · 2024-10-15T14:42:33Z

lib/graphql/batch/loader.rb

@@ -42,13 +42,14 @@ def current_executor
      end
    end

-    attr_accessor :loader_key, :executor
+    attr_accessor :loader_key, :executor, :deferred


I'm concerned with deferred terminology conflating with GraphQL defer, to which this isn't directly related. Can this remain consistent with the "async" terminology, or be expressed on some kind of lazy priority scale?

Yes, I agree. I'd like to use yield and yielded, which is what https://graphql-ruby.org/dataloader/parallelism uses, but that's because it uses fibers. We're conceptually "yielding control" here too.

WDYT @gmac @swalkinshaw @amomchilov ?

Since this is an internal implementation detail now, why not just call it async? And it's more consistent with the Async module naming.

We could even get rid of this entirely and just filter out loaders that don't include GraphQL::Batch::Async? (though maybe that's worse for perf)

We can, though I think that's a worse API. I think yield is a nice fundamental API, that can be used beyond Async. Async builds on top of that (and the "perform on any wait thing).

Can you put a commit up showing what it looks like?

tgwizard · 2024-12-19T15:19:01Z

I won't have time to test this out before my last day at Shopify (tomorrow). Leaving it hanging here for someone to pick up or close.

tgwizard force-pushed the properly-async-and-deferred-loaders branch from fe9b4aa to 4027212 Compare October 6, 2024 11:39

WIP: Async and deferred loaders

046a00b

tgwizard force-pushed the properly-async-and-deferred-loaders branch from 4027212 to 046a00b Compare October 7, 2024 07:45

swalkinshaw reviewed Oct 7, 2024

View reviewed changes

amomchilov reviewed Oct 7, 2024

View reviewed changes

tgwizard force-pushed the properly-async-and-deferred-loaders branch from f5ee2ab to 864d9af Compare October 14, 2024 11:38

Add GraphQL::Batch::Async

50227fe

tgwizard force-pushed the properly-async-and-deferred-loaders branch from 864d9af to 50227fe Compare October 14, 2024 11:39

gmac reviewed Oct 15, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

WIP: Async and deferred loaders #173

WIP: Async and deferred loaders #173

tgwizard commented Oct 6, 2024 •

edited

Loading

swalkinshaw Oct 7, 2024

tgwizard Oct 8, 2024

swalkinshaw Oct 7, 2024

tgwizard Oct 8, 2024

swalkinshaw Oct 8, 2024

tgwizard Oct 14, 2024

amomchilov left a comment

amomchilov Oct 7, 2024

amomchilov Oct 7, 2024

amomchilov Oct 7, 2024

gmac Oct 15, 2024

tgwizard Oct 16, 2024

swalkinshaw Oct 16, 2024

tgwizard Oct 16, 2024

swalkinshaw Oct 16, 2024

tgwizard commented Dec 19, 2024

	fulfill(operation, futures[index].value!) # .value is a blocking call
	fulfill(operation, futures[index].value!) # .value! is a blocking call

	def perform_on_wait(operations)
	def perform_on_any_wait(operations)

WIP: Async and deferred loaders #173

Are you sure you want to change the base?

WIP: Async and deferred loaders #173

Conversation

tgwizard commented Oct 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

amomchilov left a comment

Choose a reason for hiding this comment

General idea

Documentation considerations

Implementation considerations

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tgwizard commented Dec 19, 2024

tgwizard commented Oct 6, 2024 •

edited

Loading