2029688 - [Optimization] Determine if it is advisable to prevent double calls to getPreferenceStateFromGecko

Status: RESOLVED FIXED

(Firefox for Android :: Experimentation and Telemetry, task)

Description

[Olivia Hall [:olivia]]

When working on bug 2025031, I tested another approach that seemed plausible. The root cause of the other bug was GV could not lazily handle rapid calls to the same API in a row for that bug. However, we really shouldn't have too many API calls in a row, in general, especially around getting information.

The other approach, that I didn't do in bug 2025031, was that I gated getPreferenceStateFromGecko to only allow one in-flight call at a time and force the other threads to the same call. I think this is a viable solution to preventing multiple calls and it tested well under bug 2025031's conditions.

It would be good to reduce the amount of calls because a lot of these are redundant where different tasks are trying to get the same information, for example setGeckoPrefsState needs information from getPreferenceStateFromGecko() as well as the startup call. It seems the setter call can sometimes happen while the startup call is still inflight.

What prevented me from going ahead and limiting this call to only one in-flight call is that I'm concerned sometimes multiple calls could be legitimate if preferenceList changes in-flight.

Patch tested:
NimbusGeckoPrefHandler.kt:

+ internal var isFetchingGeckoPrefState: Deferred<Boolean>? = null  

...

      fun getPreferenceStateFromGecko(): Deferred<Boolean> {
          val completable = CompletableDeferred<Boolean>()
          geckoScope.launch {
 +            // Return the existing call, if one is already in-flight.
 +            val existingFetch = isFetchingGeckoPrefState
 +            if (existingFetch != null) {
 +                completable.complete(existingFetch.await())
 +                return@launch
 +            }
 +            isFetchingGeckoPrefState = completable
 +
              try {
                  engine.value.getBrowserPrefs(
                      prefs = preferenceList,
...
                             }.toString()
                              state.isUserSet = preference.hasUserChangedValue
                          }
 +                        isFetchingGeckoPrefState = null
                          completable.complete(true)
                      },
 -                    onError = { completable.complete(false) },
 +                    onError = { throwable ->
 +                        logger.error("Error getting preference state from Gecko", throwable)
 +                        isFetchingGeckoPrefState = null
 +                        completable.complete(false)
 +                    },
                  )
              } catch (e: IllegalThreadStateException) {
                  logger.error("Error getting preference state from Gecko", e)
 +                isFetchingGeckoPrefState = null
                  completable.complete(false)
              }
          }

It would be a good follow-up to see if this is a viable optimization.

Comment 1

[Olivia Hall [:olivia]]

Other options to properly sharing preferences state would be good as well.

Comment 2

[Olivia Hall [:olivia]]

Attachment: Bug 2029688 - Prevent double calls when possible

Added logic to check for duplicate checks for the same information in
getPreferenceStateFromGecko to help limit unnecessary calls.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/98bf55ff047e
https://hg.mozilla.org/integration/autoland/rev/70ca61a8bc51
Prevent double calls when possible r=android-reviewers,mcarare

Comment 4

[Cosmin Sabou [:CosminS]]

https://hg.mozilla.org/mozilla-central/rev/70ca61a8bc51