made subscription broadcast multithreaded

[Shubham8287]

Description of Changes

Made broadcast_commit_event multithreaded to leverage read locks.

[coolreader18]

I think right now, I mostly don't see the point of this change? Is there a plan to start calling these methods from other threads in the future? Because with the current changeset, I don't see any real operational difference. Is the goal to run multiple ModuleSubscriptionActor commands in parallel? begin_tx() in broadcast_commit_event takes a lock on the datastore anyway, so it would still run in serial.
I'm not sure if this is a good idea as implemented here, since theoretically commands could run out-of-order from the order they were sent, which is bad. Oh also - I think I just realized, the deadlock we're getting is possibly write-starving; the read() locks collectively holding on to the rwlock constantly, with the write() waiter stuck forever.

[joshua-spacetime]

begin_tx() in broadcast_commit_event takes a lock on the datastore anyway, so it would still run in serial.

It takes a shared lock now.

commands could run out-of-order from the order they were sent

@coolreader18 is correct. If txn A runs(commits) before txn B, then we need to ensure that broadcast_commit_event is called on A before it is called on B. Unfortunately we'll introduce inconsistency if we multithread subscriptions apart from their originating transactions.

[joshua-spacetime]

Requesting changes because I think we can only multithread the initial subscription calls, not the incremental evaluations unfortunately. At least not under the current concurrency model.

[joshua-spacetime]

This function really shouldn't be async. The reason being is that subscription evaluations need to have a serial equivalent ordering to the original transactions.

If txn A inserts a row, and then txn B deletes that same row, it would be incorrect to evaluate the effects of B before the effects of A.

Note this does not necessarily apply to the initial subscribe call eval.

[Shubham8287]

I got you, This would require some more work I guess. Thanks for pointing.

[joshua-spacetime]

@Shubham8287 just reiterating the solution that we discussed earlier:

If each reducer holds an exclusive lock on the ModuleSubscriptionManager, we can evaluate all of the subscription queries for single transaction concurrently, and at the same time preserve the ordering across transactions.

That is, reducers are still run sequentially and do not complete until all of the required subscription queries have been evaluated. However within said reducer, queries can be run in parallel.

[Shubham8287]

Thanks for summarizing.
I have done changed the we intented to achive parrallelism, so that now instead of trying to run broadcast_commit_event parallelly. The method will spawn threads to evaluate each Subsription QuerySets
and keeping subscription actor itself single threaded.

[Shubham8287]

I don't see any real operational difference. Is the goal to run multiple ModuleSubscriptionActor commands in parallel? begin_tx() in broadcast_commit_event takes a lock on the datastore anyway, so it would still run in serial.

begin_tx() holds only read lock, so we can have parallel broadcast_commit_event executions.

the deadlock we're getting is possibly write-starving; the read() locks collectively holding on to the rwlock constantly, with the write() waiter stuck forever

this could be true.

[jdetter]

I've tested this thoroughly using the bots and I'm not seeing the deadlock that we had before. The performance here is really good so let's try to get this in asap 👍

[joshua-spacetime]

Feel free to merge @Shubham8287. The consistency issue can be handled separately #758.