Page MenuHomePhabricator

[native] Increase the number of threads available for rust
ClosedPublic

Authored by michal on Nov 27 2023, 5:41 AM.
Tags
None
Referenced Files
F2897415: D10055.diff
Fri, Oct 4, 11:20 PM
Unknown Object (File)
Sep 3 2024, 6:17 PM
Unknown Object (File)
Sep 3 2024, 6:16 PM
Unknown Object (File)
Sep 3 2024, 6:16 PM
Unknown Object (File)
Sep 3 2024, 6:16 PM
Unknown Object (File)
Aug 26 2024, 11:44 PM
Unknown Object (File)
Aug 10 2024, 8:04 PM
Unknown Object (File)
Jul 12 2024, 9:11 AM
Subscribers

Details

Summary

ENG-5561

We don't want to block other tasks on backup operations. We can increase the number of used thread in tokio runtime to the default (the number of cores of the system) so other operations can still proceed. This shouldn't cause a problem for NSE (there were worries that too many threads could make the notifications less likely to be delivered) because rust_native_library isn't even compiled for the notification service.

Test Plan

Test that the app runs on physical iOS and Android devices

Diff Detail

Repository
rCOMM Comm
Lint
No Lint Coverage
Unit
No Test Coverage

Event Timeline

Harbormaster returned this revision to the author for changes because remote builds failed.Nov 27 2023, 7:21 AM
Harbormaster failed remote builds in B24507: Diff 33711!
marcin added inline comments.
native/native_rust_library/src/lib.rs
42

Treat this comment as a food for thought rather than something you should strictly answer before proceeding with this diff.

  1. Threading libraries in Java ecosystem allow to create threads in an adaptive manner as they are needed (https://www.baeldung.com/java-executors-cached-fixed-threadpool). Is it possible to do something similar with tokio? Perhaps it could be better with mobile app where resources are limited.
  2. Is it possible that second backup operation will be scheduled before previous one completes? If so then running them in parallel might cause bugs.
  3. Are there any other operations that might interfere with backup operations that will run in parallel with backup once we enable multithreading.
This revision is now accepted and ready to land.Nov 28 2023, 7:02 AM
native/native_rust_library/src/lib.rs
42

For 1,2 I feel like this is a higher-level issues and will be largely solved at the js-level. If we are in restoration mode, the app can't really do anything else, this is a blocking operation. Running backup upload operations should be synchronized with other app operations by either using the GlobalDBSigleton or other synchronization methods (e.g. previously we were thinking about using files for log synchronization).

For 1 tokio exposes something called blocking threads, spawn_blocking. In particular:

performing a lot of compute in a future without yielding is problematic, as it may prevent the executor from driving other futures forward

So we could put the encryption operations inside it, especially when they become larger (right now, for the testing code it's just user store). One issue is that they are mostly to spawn *from async* code and not the other way around. I think it might make to wait for the rest of the networking client to decide how to use them exactly (as networking will be async-based) to see how the code will be structured.