perf: increase task throughput in Android using thread pool executor

[kmsbernard]

Description

Related discussion: #4969

This PR aims to increase task execution throughput in Android with making minimal behavioral changes.

• As before, transactional tasks execute with single-threaded executor.
  • This includes "write" actions such as set, update, remove at the database/firestore and putFile at the storage.
• Non-transactional tasks execute with configured ThreadPoolExecutor.
  • If thread pool size exceeded, the task execute with the single-threaded executor which has unbounded queue size.
  • configuration reference: Android AsyncTask module

I haven't run precise benchmarks yet, but typical use case applications should have improved performance.

further considerations:

• Is transactional tasks cleanly, and correctly divided?
• Should transaction scenario cover with test?

Checklist

• I read the Contributor Guide and followed the process outlined there for submitting PRs.
  • Yes
• My change supports the following platforms;
  • Android
  • iOS
• My change includes tests;
  • e2e tests added or updated in packages/\*\*/e2e
  • jest tests added or updated in packages/\*\*/__tests__
• I have updated TypeScript types that are affected by my change.
• This is a breaking change;
  • Yes
  • No

Test Plan

---

Think react-native-firebase is great? Please consider supporting the project with any of the below:

• 👉 Star this repo on GitHub ⭐️
• 👉 Follow React Native Firebase and Invertase on Twitter

[vercel]

This pull request is being automatically deployed with Vercel (learn more).
To see the status of your deployment, click below or on the icon next to each commit.

🔍 Inspect: https://vercel.com/invertase/react-native-firebase/9mE6AeHakSHddrBbQfBngMVUEuF2
✅ Preview: https://react-native-f-git-fork-bernard-kms-thread-pool-executor-47b2c7.vercel.app

[codecov]

Codecov Report

Merging #4981 (7fd6d72) into master (a61d0cd) will decrease coverage by 0.12%.
The diff coverage is 20.00%.

❗ Current head 7fd6d72 differs from pull request most recent head 50e6903. Consider uploading reports for the commit 50e6903 to get more accurate results

@@            Coverage Diff             @@
##           master    #4981      +/-   ##
==========================================
- Coverage   89.11%   88.99%   -0.11%     
==========================================
  Files         109      109              
  Lines        3734     3739       +5     
  Branches      350      358       +8     
==========================================
  Hits         3327     3327              
- Misses        365      367       +2     
- Partials       42       45       +3     

[mikehardy]

Hoping to look at this today or tomorrow - please have patience if it takes a little bit, but thank you very much for posting this

I will mention that on all PRs we generate patch-package patch sets for the PR as artifacts, you can use those in the meantime if you like:

here is the patch-package CI job https://github.com/invertase/react-native-firebase/pull/4981/checks?check_run_id=2021948264

and the artifact (not sure if the URL is stable) https://github.com/invertase/react-native-firebase/suites/2168204522/artifacts/44535863

[kmsbernard]

As @Salakar pointed out in the discussion thread(#4969 (reply in thread)), realtime update event(supported in realtime database and firestore) should be handled in serial per query, which is currently(in this PR) not. This will be addressed in further works.

[mikehardy]

Ok! I'll wait for real review until that is done but I can still give this a scan for structural items etc and have that queued this morning. I think this will be a big performance benefit if the special cases can be handled and am still positive on the idea, for what it's worth :-)

[kmsbernard]

Now it processes realtime event in serial per listener. The approach taken here is to separate each event listener into a single threaded executor.

When a new listener is created, new single threaded executor is created and registered with the listener identifier(eventRegistrationKey in database and listenerId in firestore). And when it's unsubscribed, the executor is removed accordingly.

[mikehardy]

Hi there! I just read through this and thought about it this morning - not ready for approval yet - I left some comments (some prescriptive, some more questions...) in various spots

Allowing the executor limit to be tunable in particular, with the ability to have the whole system go back to a single thread if needed is important to me in order to allow users to self-heal if there are problems. In fact the first version might ship with it tuned to 1 in order to not change the default without opt-in field testing first

Here is a PR that implements a firebase.json feature: https://github.com/invertase/react-native-firebase/pull/4870/files - then I think with it in AndroidManifest.xml you may access the value in code and use it for pool size tuning?

[mikehardy]

On re-read my comment seems 100% critical whereas it's important to recognize this is quite the fancy piece of work in my opinion. I think it's clever, and it's actually pretty close to merge, and should help a lot of people, and that's worth saying :-). Cheers.

[kmsbernard]

Thanks for the careful review!

In fact the first version might ship with it tuned to 1 in order to not change the default without opt-in field testing first

Indeed, I think it's important to not make any breaking changes in the first release of this improvement.

I will add commits to implement tunable pool sizing, and addressing review comments.

[Salakar]

Pending the points by @mikehardy, this seems like a nice approach and looks great - thanks for this @bernard-kms

[mikehardy]

Thank you again for your patience, this month has been busier than I thought. It's worth stating again I'm really excited about this.

Also, the code and documentation are excellent.

The only thing I can think of at all is maybe a need for synchronizing on the executors object to protect against concurrent access during gets that result in sets, or gets / shutdowns / removes happening at the same time

I'm curious for your thoughts there.

That said, I'm going to integrate this patch set into my work app for a test drive [Edit: it's running fine in my work app with 10/3 per documentation]

[kmsbernard]

I agree with your concern, and added a commit to prevent race condition during getting/setting executors.

I also ran a test track with my app, and there seems to be no problem.

[mikehardy]

I just re-read the whole thing again but with a focus on the changes of course. Still looks good to me, and I have this integrated with my work project as a set of patches, working well also. I think it's good to go! 🚀 - I think this will be a big help to some people @bernard-kms - thanks

[mikehardy]

This finally launched in v11.3.0 just now! Thanks for your patience and for making this change it should be a big help

[kmsbernard]

Good! Thanks for the hard work to maintain this wonderful library. It's my pleasure to be a contributor :)

[zholmes1]

@mikehardy @bernard-kms

Do either of you think that this upgrade would fix this intermittent C++ crash that some of my users are experiencing when storage.putFile is called?

SIGABRT 0x0000000000000000
libc.so
... 61 more ...
libcutils.so

My app is currently on rnfb@10.5.1, and I'm considering upgrading everything to 12.0.0 as a possible way to address this issue. Alternatively, I could do away with storage.putFile altogether and create a function that accepts the image in an HTTPS request, and uploads the file to storage from the Node function code. Although that brings with itself other potential points of failure. So I'd like to stay with the storage package, if possible.

Any help would be appreciated, and looking at the changelog for the storage package, this is the only PR that would potentially address my issue.

[mikehardy]

@zholmes1 commenting on random pull requests is odd from a maintainer's perspective. Your question is not related to the PR at all really

If I were suffering from a native crash I would always update to current stable before doing anything, literally before even looking at the logs again, as it's likely a waste of time because either it's fixed or (like us) anyone you ask about will just say "update then reproduce then ask again", because it could be fixed.

If you only consult our change logs here in react-native-firebase you're potentially missing any changes from underlying firebase-ios-sdk / firebase-android-sdk and all of their transitive dependencies (a lot) all of which move at a steady pace and have been updated between your version and current stable. Any of them could have fixed it

[zholmes1]

Well it's not really random, since it's the only applicable commit between my version and the current version.

I'm just concerned about potential regression, as I think we all have experiences with that when upgrading packages. But good point about the issue possibly being in the Android SDK, I hadn't considered that. I'll upgrade to 12.0.0 and if I still see this crash, I'll open an issue.

Thanks for the quick reply.

[mikehardy]

the commits that bump SDK versions are all applicable as is anything in @app really but I also see your point if you're thinking super narrowly on storage.

[zholmes1]

True, I honestly wan't even thinking about bumping SDK versions. Benefit of using awesome cross-platform packages like this one!