Reswift: Slow subscribe ?

[nferruzzi]

Hi,

I've a project with thousands of subscribers, the more I add the slower it becomes to add new ones.
I think the main reason is here

        if let index = subscriptions.index(where: { $0.subscriber === subscriber }) {
            subscriptions.remove(at: index)
        }

unfortunately this is going to be a major issue for my project; would you consider a PR for a different approach like an hashtable or whatever ?

thanks in advance

[DivineDominion]

Thousands (plural!!) sounds crazy. Maybe it's just me and my approach is old-fashioned :)

Using a Set (and thus Hashable) should indeed improve performance. We have to be careful to not just provide a quick fix that makes the public API worse. It's fairly simple to wrap subscription blocks in StoreSubscriber at the moment, but if someone changed their subscription to block-based this way, how should they provide the hash value? An auto-incrementing integer would be a start, but also very odd.

I cannot say I like the change much, but I also cannot come up with good reasons to keep the subscription/removal algorithm as slow as it is now. Even though it sounds weird to me, why not support thousands of subscribers? I cannot come up with a real reason to not support this besides "breaking API change".

Although I wonder how the performance of propagating state updates is for you :)

[nferruzzi]

Thanks for starting a discussion :)

I don't have much to add to my bug report, unfortunately I lack the confidence to talk about a solution yet but I can point out that the main problem for me is the price to pay for indexing the array of subscriptions when I can guarantee my subscribers are all unique. A "patch" for my case on my local fork is to have a "bulk creation" flag to toggle the index test and live with that until an improvement is done.

Still, I've a question, why do you think this requires a breaking API change? I'm new to the project and just started exploring the code base but I believe it would be better to keep the same generic api but also support something like Hashable StoreSubscribers with a new class of subscribe functions
func subscribe<S: StoreSubscriber & Hashable> ...

I understand it would split the code path to support hashable and non hashable subscribers but this would leave the problem of "what is the proper hash for your subscriber?" in the hand of the user where it should actually be.

Although I wonder how the performance of propagating state updates is for you :)

it's a linear time and that is ok :)

[mjarvis]

Since we restrict subscriptions to class types, we might be able to do some object identifier magic for a hash table. Let me see if I can whip something up.

[mjarvis]

Yeah I think that might work a little better :)
Opening a PR in a moment

[Dimillian]

Interesting, still need to understand a pattern where you have thousands of subscribers. I have a game which manage thousands of units using ReSwift, subscribing and dispatching to all the units would be indeed slow. (Didn't even tried, but I guess that dispatching to thousand of subscriber would be even more slower than subscribing). I decided first to do a controller around units of the same type, so this controller actually subscribe to the state, and dispatch the change to the units which are owned by this controller.

I preferred to do a controller around anyway, because there was much more to handle than state changes.

In an iOS app (not a game), I would love to see a use case using a lot of subscribers, I need to try it myself.

I wonder how React coupled with redux handle that, as the recommended design pattern is to not do your state connect not too deep.

[mjarvis]

After a bit more testing it appears that changing subscriptions to a set causes a significant (46%) regression to state update performance, due to the increased cost of filtering & enumerating a set. I'm taking a look to see if theres anything I can quickly do to reduce that cost, but I'll be PR'ing this change either way to allow others to change things as well, however I recommend we don't merge the PR unless we can resolve the state update performance regression, as for most users that is a much more important metric.

[mjarvis]

Managed to get both the subscribe & notify performance better.
See the PR for details, but in summary, when subscribing and notifying 3000 subscribers:

• testNotify(): 0.0046359 -> 0.0031482 (32% faster)
• testSubscribe(): 0.1861 -> 0.0087853 (95% faster)

[nferruzzi]

guys you are just awesome! thanks for the top class support 👍

[brownsoo]

👍

[DivineDominion]

Amazing work @mjarvis