Coalesce all merged run effects into a single task group.

[klundberg]

I noticed that if one were to .merge many .run effects, that it would create as many task groups as there were effects in the list of effects (minus 1). I suspected this may use more memory than needed, so I tried coalescing all run effects into a single task group to save on memory usage.

I ran something like the following code on the simulator both with and without my change to test my hypothesis, in an arbitrary reducer:

case someAction:
  var fx: [EffectOf<Self>] = []
  for _ in 1...1000 {
    fx.append(.run { send in try await Task.sleep(for: .seconds(1)) })
  }
  return .merge(fx)

I'm using a lot of tasks here to make the memory usage differences much more obvious in the memory used metrics in xcode's debugging navigator.

When putting that code into a screen in the swiftui case studies app, without my change the app used approximately 43 MB of memory while the tasks were active, and with my change it used 41.5 or so MB of memory while the tasks were active. This shows that there could be some memory savings gained here especially if some effects in a large group of merged effects are long-lived or if one of them never ends.

Two things I can think of that might be concerns here are the order that effects are initiated is different here; since run effects are coalesced together, if theyre mixed with publisher effects they won't initially trigger in the same order as before this change. The second thing is wrapping the some Sequence<Self> in an unchecked sendable to pass it along to the task group may not be safe. We could create an interim array instead if doing so isn't a concern, but for pathological cases like my test case that may take extra time. If there's little or no concern of assuming that any sequence used here could be safely considered sendable though then it may work fine.

If there's any concerns with this change I can address let me know!

[stephencelis]

@klundberg Sorry this fell off our radar. Will chat about it with Brandon soon!

[stephencelis]

@klundberg Took the time to look at this today and while it's definitely touching on some things we want to improve in the future, it's unfortunately a pretty tough thing to change right now, and unfortunately your implementation is a breaking change.

First, forcing things into a task group means existing .publisher-only effects that ran synchronously will now incur a thread hop, and it means other .run effects may run in a different order than before.

Second, while your benchmark shows an improvement, I think it's important to note that the average number of non-.none effects spun up by sending an action to the store is probably <1 for most apps (as opposed to 1,000), and so we think the memory gains in most real world apps would be quite small.

The good news is we have some substantial improvements planned for effects in 2.0, which include spinning them up inside a discarding task group by default. This means we won't even need to create unstructured tasks per effect. So please bear with us as we get things in order for a 2.0 later this year 😄

I'm going to close this out for now, but if there's anything else you want to chat about, please open a discussion!