(Originally filed as bug 1620105)

Profile: https://perfht.ml/2VN7meO (2ms interval) https://perfht.ml/2ImRyrj (10ms interval)

In this profile of GeckoView startup, SafeBrowsing.readPrefs() takes up ~170ms: First, it takes ~40ms during SafeBrowsing.init(), and then ~130ms in response to various prefs being set.
We should find out whether it makes a difference to set these prefs before GeckoViewStartup calls SafeBrowsing.init(). From a brief glance at the code, it looks like the 130ms are pure repetitive work that be eliminated entirely.

Snorp has filed https://github.com/mozilla-mobile/android-components/issues/6193 on this.

So between the previous incarnation of this and the a-c bug we should be taking care of the 130ms bit. Do we want to track the 40ms in SafeBrowsing.init() here, or is it something else?

Hmm, what I really want is an entity in bugzilla that represents the a-c bug, so that it will show up in the list of bugs that block bug 1550073.
I'm fine with resolving this bug as MOVED if we're sure that the a-c bug will fix the problem.

I don't think we need a bug for the 40ms in init(); those 40ms might be a little higher than desired but there are probably bigger fish to fry.

P5 because we have no work to do here, just tracking the a-c bug.

The A-C issue has been closed with a hint at something that landed in Fenix. However, with a Fenix build from Fenix master as of today, I still see the extra 130ms in Safebrowsing code: https://perfht.ml/3aaEIsB

This seems to be caused by the prefs in ContentBlocking being set after startup. Dylan, can we move these to runtime settings? If we do that, we'll set the defaults during Gecko startup, avoiding the SafeBrowsing.readPrefs() penalties.

(In reply to James Willcox (:snorp) (jwillcox@mozilla.com) (he/him) from comment #5)

This seems to be caused by the prefs in ContentBlocking being set after startup. Dylan, can we move these to runtime settings? If we do that, we'll set the defaults during Gecko startup, avoiding the SafeBrowsing.readPrefs() penalties.

Let's chat about this tomorrow. If possible, I'd prefer to keep them where they are (for organizational reasons if nothing else) and fix the underlying issue.

This still looks like a Fenix/AC issue to me -- if you provide the ContentBlocking.Settings at GeckoRuntime.create() you should not hit any unnecessary pref changes. GVE does this:

            runtimeSettingsBuilder
                    .useMultiprocess(useMultiprocess)
                    .remoteDebuggingEnabled(mEnableRemoteDebugging)
                    .consoleOutput(true)
                    .contentBlocking(new ContentBlocking.Settings.Builder()
                        .antiTracking(ContentBlocking.AntiTracking.DEFAULT |
                                      ContentBlocking.AntiTracking.STP)
                        .safeBrowsing(ContentBlocking.SafeBrowsing.DEFAULT)
                        .cookieBehavior(ContentBlocking.CookieBehavior.ACCEPT_NON_TRACKERS)
                        .enhancedTrackingProtectionLevel(ContentBlocking.EtpLevel.DEFAULT)
                        .build())
                    .crashHandler(ExampleCrashHandler.class)
                    .preferredColorScheme(mPreferredColorScheme)
                    .telemetryDelegate(new ExampleTelemetryDelegate())
                    .aboutConfigEnabled(true);

            sGeckoRuntime = GeckoRuntime.create(this, runtimeSettingsBuilder.build());

and it appears to work fine. By contrast, Fenix creates the GeckoRuntime here: https://searchfox.org/mozilla-mobile/source/fenix/app/src/geckoBeta/java/org/mozilla/fenix/engine/GeckoProvider.kt#45 and notably does not include a contentBlocking in the runtimeSettings passed to GeckoRuntime.create(). I'll reopen the AC issue.

(In reply to Dylan Roeh (:droeh) (he/him) from comment #7)

I'll reopen the AC issue.

I opened a Fenix issue, as the actual runtime creation was happening in their code: https://github.com/mozilla-mobile/fenix/issues/10788. Also moving this to p5 to track the issue, I don't think there's anything we can do on GV's side here.

Markus, I'm trying to review the change to make sure it had the desired effect but I don't know what to look for. I took a profile before and after the change and posted them here: https://github.com/mozilla-mobile/fenix/issues/11333#issuecomment-668871868

Can you tell me what I need to look for in the profile to validate that this fix worked or just go ahead and validate the fix yourself? Thanks!

Looking at the profiles in the comment you linked to, it seems like it worked - in the new profile, readPrefs is only called under init, it is no longer called under Preferences.set. So it looks like we're no longer re-initializing SafeBrowsing repeatedly.
And the number of samples in SafeBrowsing code during startup has been reduced from 37 to 10. Those extra 27 samples (27 samples * profiling interval of 5ms = 135ms) are exactly the extra 130ms that I filed this bug about.

Nice work!

Given the downstream issue in fenix is closed https://github.com/mozilla-mobile/fenix/issues/11333, can we close this this issue as well?

(In reply to Michael Comella (:mcomella) [needinfo or I won't see it] from comment #11)

Given the downstream issue in fenix is closed https://github.com/mozilla-mobile/fenix/issues/11333, can we close this this issue as well?

Yup.