Re-implement getExecutor() method for better performance and efficiency?

[kmsbernard]

Hi!

I'm struggling with an issue that firestore query response is about 4x slower on Android than on iOS, in some cases. And I found out that the library handle async job with Tasks and Executor on Android:

react-native-firebase/packages/app/android/src/reactnative/java/io/invertase/firebase/common/ReactNativeFirebaseModule.java

Line 76 in f57d69d

public ExecutorService getExecutor() {

It seems to create single-thread executor for each module(auth, firestore, etc). But Android reference says use of a cached thread pool will typically improve the performance of programs that execute many short-lived asynchronous tasks: https://developer.android.com/reference/java/util/concurrent/Executors#newCachedThreadPool()

I locally modified getExecutor() to create a CachedThreadPool, and it made the performance much better. My app fetches about 8 queries at the same time during initial rendering, so I think my scenario is most affected one.

Also, the current implementation always has fixed threads depending on the number of modules, but the thread pool kills unnecessary threads, it can be more efficient in terms of resource management.

I think changing the implementation of getExecutor() method using well-configured ThreadPoolExecutor will make the library more performant and efficient on Android: https://developer.android.com/reference/java/util/concurrent/ThreadPoolExecutor

I'm considering to make PR for this, so I'm wondering if there is a reason to create a single thread executor.

[mikehardy]

I'm very interested in this - we have had reports of query performance being slow on Android but have been unable to resolve it, you may have hit the root cause (or, "a" root cause, but perhaps a big one)

Please post the PR either way, I'd love to see it, but in general either @Salakar or @Ehesp would have to answer the architecture question of "does react-native-firebase use single-threaded Executor per module on purpose, or was it simply expedient and making it a thread pool is fine"

[mikehardy]

@bernard-kms

Here is the response from @Salakar (original author I believe, or co-author at minimum):

mainly for transactions I think, you want the actions executing in the order they were sent on DB & Firestore

so if transactions can be broken away to keep using the single executor then I'd say try the cached thread pool on everything else

Thus, if you can cleanly divide transaction-using tasks (package database, package firestore, either when a user is in a transaction?) from non-transaction tasks, then a thread pool could be a nice improvement for the non-transaction cases.

Does that seem doable?

[kmsbernard]

Thank you for your quick response!

Yeah, the first idea came into my head is to divide the executor into thread-pool and single-thread(or, something like transactional thread-pool?) and using each of them in appropriate case. Now I understand what was considered for a single thread executor, so it might be trickier than I thought. But I think I can deal with it.

[Salakar]

I've just remembered something that's worth checking in regards to real time events for DB and Firestore and the single thread executor that will need to be checked;

There was some issues with using a thread pool if I recall correctly around the ordering of say Firestore snapshot events when subscribing to a document or query for example. What happened was say the following events came through the native listener in this order at around the same time give or take a few ms:

1. DocumentSnapshot that exists and has a bunch of data and is fairly large in payload size.
2. DocumentSnapshot that does not exist and therefore has no data and is relatively small in payload size.

In a single thread executor these would be first in first out (FIFO). However with multiple threads what we found is that the smaller payload size was able to go through the RN serialisation logic we have on native quicker than the event that had more data to serialize, therefore it came through first. Leaving the app with an incorrect state of the current record on Firestore.

This only applies to event based APIs such as listeners for RTDB, Firestore and Storage though and is not an issue for standard method calls, so perhaps there can be some separation here unless there's another way of solving this.

[kmsbernard]

Oh, that's a good point. I overlooked that scenario. I think there's a way to isolate each event listener with a single thread to guarantee FIFO policy per query.

I will address this concern in the PR #4981