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

Allow dependencies initialise in parallel #96

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

DeLaGuardo
Copy link

@DeLaGuardo DeLaGuardo commented May 23, 2022

Related #22

This PR enhances the sorting strategy by grouping keys based on the depth of nodes. So instead of attempting to sequentially invoke initializing function for each element in topologically sorted nodes - perform the same for batches of keys grouped by their position in the graph.

For example, consider the following graph:

┌───┐
│ A │
└───┘
  │
  ├───────────┬───────────┐
  │           │           │
  ▼           ▼           ▼
┌───┐       ┌───┐       ┌───┐
│ B │       │ C │       │ D │
└───┘       └───┘       └───┘
  │
  ├───────────┐
  │           │
  ▼           ▼
┌───┐       ┌───┐
│ E │       │ F │
└───┘       └───┘

Before this PR integrant will try to build the system from this sequence: (E F B C D A). So if some node is taking some time to finish initialization it will block subsequent initialization attempts. With this PR the same sequence will have additional grouping: ((E F C D) (B) (A)) giving the opportunity to parallelise the initialisation step for the keys in the same group.

Here is a made-up example to illustrate the benefits:

(defmethod ig/init-key ::foo [_ timeout]
  (Thread/sleep timeout)
  nil)

(defmethod ig/init-key ::bar [_ [timeout _]]
  (Thread/sleep timeout)
  nil)

(time
 (-> {[::foo ::o] 1000
      [::foo ::p] 1000
      [::foo ::q] 1000

      ::bar [1000 (ig/refset ::foo)]}

     (ig/init)))
;; => "Elapsed time: 2005.345024 msecs"

Normally such a system will take at least 4 seconds to build because each initialisation function must finish before attempting to initialise the next node.

Those adjustments will be visible only in Clojure because the current implementation relies on clojure.core/future function which is missing in ClojureScript but probably it is possible to adjust it later.

List of difficulties to take care of (from comment):

  • The order of components starting is no longer deterministic
  • We need to build our own iteration mechanism
  • We need to keep a list of all dependencies that have begun to be initiated, as we can run into the same dependency multiple times even if the graph is acyclic
  • We need to halt all threads on an exception and return the exception
  • We might need some manner of a thread pool
  • Certain keys will take a very short time to initiate; these may actually be slower with the thread overhead

```clojure
(defmethod ig/init-key ::foo [_ timeout]
  (Thread/sleep timeout)
  nil)

(defmethod ig/init-key ::bar [_ [timeout _]]
  (Thread/sleep timeout)
  nil)

(time
 (-> {[::foo ::o] 1000
      [::foo ::p] 1000
      [::foo ::q] 1000

      ::bar [1000 (ig/refset ::foo)]}

     (ig/init)))
;; => "Elapsed time: 2005.345024 msecs"
```
@weavejester
Copy link
Owner

weavejester commented May 24, 2022

Thanks for the PR. I think this is a useful feature to have, but I think the proposed design is too complex (in the "Simple Ain't Easy" sense of the term).

Rather than try to group keys, I wonder if it would instead be better to construct a function for executing a function across a dependent graph in parallel:

(execute-graph f graph)

If we combine this with functions:

(subgraph graph keys)
(invert-graph graph)

That seems like a sufficiently simple basis to build upon, though there may very well be unforeseen problems here.

I'll take a look at this myself and try to come up with something a little more concrete, but these were my first thoughts.

@DeLaGuardo
Copy link
Author

I like the design.

Probably this situation might cause most of the problems without grouping:

┌───┐       ┌───┐
│ A │       │ B │
└───┘       └───┘
  │           │
  └─────┬─────┘
        │
        ▼
      ┌───┐
      │ C │
      └───┘
        │
  ┌─────┴─────┐
  │           │
  ▼           ▼
┌───┐       ┌───┐
│ D │       │ E │
└───┘       └───┘

I begin thinking about my implementation from graph execution with CPS pattern but this X-like situation pushed me towards grouping instead. Looking forward to seeing what you end up with.

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

Successfully merging this pull request may close these issues.

2 participants