2030021 - Avoid blocking AsyncOpen on URL classification for annotation-only features

Status: RESOLVED FIXED

(Core :: Privacy: Anti-Tracking, task)

Description

[Andrew Creskey [:acreskey]]

I don't have a deep understanding of our URL classification requirements, but I'm wondering whether or not we need to classify first party sub resources.

Each classification that's not short circuited dispatches to the URL Classifier background thread for table lookups, then dispatches back to the main thread to proceed with the connection.

This profile shows the first party subresource runnables being processed on the URL classifier thread (www.amazon.ca navigation).
https://share.firefox.dev/4cbvJbF

Numerous verbose URL classification markers added.
Binary here.

I can see that Content Classifier Engine exits early on first party subresources. (comment does not match code).
(although the runnables have already been created and dispatched to the URL classifier thread).

While AsyncChannelClassifier::FeatureTask::DoLookup() is processing the requests.
https://searchfox.org/firefox-main/rev/187dc6baf299450446944dc90ba2239913004ba8/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp#732-749

If we could indeed skip classification of first party sub resources it would reduce the number of runnables, thread hops, and to some degree, latency.

Comment 1

[Andrew Creskey [:acreskey]]

Tim, Benjamin, Emma - per our discussion, would you be able to confirm if we need to run the async url classification on first party sub resources?

As far as I can tell, for Safebrowsing we already skip sub resources in NS_ShouldClassifyChannel().

And the Content Classifier only runs on third party resource classification.

So, at least though NS_ShouldClassifyChannel(), I believe it's only the NS_ShouldClassifyChannel(this, ClassifyType::ETP) calls in nsHttpChannel::AsyncOpenFinal that are creating the async url classifications for first party sub resources.

Comment 2

[Tim Huang[:timhuang]]

In the current implementation, we skip creating URLClassifier blocking features for first-party sub-resources. However, we still need to run the URL classification on them for annotation. We also annotate first-party tracking flags, but I don't think any privacy feature uses them; perhaps some UI code does. I need to double check.

So, I think we can not block AsyncOpen for first-party sub-resources and annotate in a later loading phase.

Comment 3

[Benjamin VanderSloot [:bvandersloot]]

Unfortunately, UrlClassifierFeatureHarmfulAddonProtection is not third-party only. Though, maybe that is an acceptable restriction? @baku: do you know if this is the case, or have thoughts on this perf improvement?

Comment 4

[Andrea Marchesini [:baku]]

The harmful add-on protection blocks first-party requests generated by add-ons. A better approach would be to ask the URL-Classifier features if asyncOpen must be blocked or not. For annotation features maybe the answer is no. For the harmful add-on feature, it can check if the request comes from an extended principal.

Comment 5

[Andrew Creskey [:acreskey]]

Thank for you input -- I've renamed the bug to make it more specific. Feel free to modify it.

Comment 6

[Andrew Creskey [:acreskey]]

In terms of performance impact, there could be measurable improvements, depending on how the site was structured and hosted.

This try push adds a series of url classification markers including one, [UrlClassify] WaitTime that reports how long we waited for classification.

On www.apple.com I see a series of first party critical resources like .css and fonts that generally wait a few ms, up to 6ms for the longest one.
Not huge delays individually, but in total it adds up.
https://share.firefox.dev/4ck3zez

Comment 7

[Andrew Creskey [:acreskey]]

Loading www.ikea.com on Fenix is a good example of a top site we could make faster if we could defer annotations for first party subresources:
https://share.firefox.dev/47WJ2vj

I can see about 18 early resources that each wait over 20ms, and about 15 resources that wait between ~10ms to 19ms

I imagine that the improvement from skipping the blocking would be roughly equal to the duration of the longest classification delay for a critical resource.

So probably something like 27ms-28ms for this navigation (on my Samsung A54), based on the delays for these resources:

Livingrrom_16, 28.0ms
PAX_startpage_poster, 27ms

Comment 8

[Benjamin VanderSloot [:bvandersloot]]

Yeah, this should be a nice addition. We are working on some major revisions to the channel classifier already, but I'm willing to put the time in now to fix this in the pre-revision state and make sure we don't regress it going forward. This isn't just a few lines of diff, but it should be alright.

I'm planning to add a boolean member function to all of the classifier rules that says whether it may block the channel. Then only grab those ones in the existing AsyncOpen callsite. The rest of them I will kick off async further down the connection initialization. I know the Safebrowsing classification suspends the channel in BeginConnect and resumes it after classification, so maybe something of that structure would be good. I wonder if that is also blocking the request's progress before we send headers.

Comment 9

[Andrew Creskey [:acreskey]]

(In reply to Benjamin VanderSloot [:bvandersloot] from comment #8)

Yeah, this should be a nice addition. We are working on some major revisions to the channel classifier already, but I'm willing to put the time in now to fix this in the pre-revision state and make sure we don't regress it going forward. This isn't just a few lines of diff, but it should be alright.

I'm planning to add a boolean member function to all of the classifier rules that says whether it may block the channel. Then only grab those ones in the existing AsyncOpen callsite. The rest of them I will kick off async further down the connection initialization. I know the Safebrowsing classification suspends the channel in BeginConnect and resumes it after classification, so maybe something of that structure would be good. I wonder if that is also blocking the request's progress before we send headers.

Great news -- I'm happy to help quantify the impact of any possible solutions :)

Comment 10

[Benjamin VanderSloot [:bvandersloot]]

For posterity:

I wonder if that is also blocking the request's progress before we send headers.

it does not- suspension doesn't seem to stall the channel before OnStartRequest (when we have data from the server)

Comment 11

[Benjamin VanderSloot [:bvandersloot]]

Attachment: WIP: Bug 2030021 - first party channels don't need to block issuing the connection - WIP

This is early WIP soon, and is likely to change.

1. I don't like exactly how the refactor shook out.
2. We need to make sure that suspending a channel in OnBeforeConnect
   allows the channel to send data before its resumption.
3. I need to double check the logic in the new annotation-only path in
   SerialChannelClassifier
4. Annotations from the new engine should be moved over too

Comment 12

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 1 - Split classifier features into those that affect blocking and don't - r=#anti-tracking!

Comment 13

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 2 - Refactor FeatureTask creation to allow creation with different features - r=#anti-tracking!

Comment 14

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 3 - Limit AntiFraud and ConsentManager annotations to third-party channels - r=#anti-tracking!

Comment 15

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 4 - Clean up invocations of NS_ShouldClassifyChannel for ETP - r=#anti-tracking!

Comment 16

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 5 - Add a nimbus controlled preference to control annotation deference - r=#anti-tracking!

Comment 17

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 6 - Split channel checking in AsyncUrlChannelClassifier to move annotations to a later call site - r=#anti-tracking!

Comment 18

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 7 - Allow network request to proceed while annotating trackers - r=#anti-tracking!

Comment 19

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 8 - Add telemetry and a Nimbus feature for anti-tracking channel classification - r=#anti-tracking!

Comment 20

[Benjamin VanderSloot [:bvandersloot]]

Attachment: Bug 2030021, part 9 - Add tests for deferred annotation - r=#anti-tracking!

Comment 21

[Andrew Creskey [:acreskey]]

I was watching this bug with the intention of jumping in and adding telemetry to measure the classification time but it looks like you've already done that Benjamin. :)

Comment 22

[Benjamin VanderSloot [:bvandersloot]]

Glad to take it off your plate. Feel free to give a drive by review if you have any different measurements!

Comment 23

[Pulsebot]

Pushed by [e-mail]:
https://github.com/mozilla-firefox/firefox/commit/1307fd9f28de
https://hg.mozilla.org/integration/autoland/rev/f7c35b75ca12
part 1 - Split classifier features into those that affect blocking and don't - r=timhuang
https://github.com/mozilla-firefox/firefox/commit/2c3599c39c1f
https://hg.mozilla.org/integration/autoland/rev/9cc558447e22
part 2 - Refactor FeatureTask creation to allow creation with different features - r=timhuang
https://github.com/mozilla-firefox/firefox/commit/eec2871b6a5f
https://hg.mozilla.org/integration/autoland/rev/287bc0c6367f
part 3 - Limit AntiFraud and ConsentManager annotations to third-party channels - r=timhuang
https://github.com/mozilla-firefox/firefox/commit/a8ee98065531
https://hg.mozilla.org/integration/autoland/rev/3eb414679ebd
part 4 - Clean up invocations of NS_ShouldClassifyChannel for ETP - r=necko-reviewers,timhuang,jesup
https://github.com/mozilla-firefox/firefox/commit/1660974b2df4
https://hg.mozilla.org/integration/autoland/rev/9647c9aeda78
part 5 - Add a nimbus controlled preference to control annotation deference - r=timhuang
https://github.com/mozilla-firefox/firefox/commit/98655dd7cd3d
https://hg.mozilla.org/integration/autoland/rev/462e30702d71
part 6 - Split channel checking in AsyncUrlChannelClassifier to move annotations to a later call site - r=timhuang
https://github.com/mozilla-firefox/firefox/commit/8ad781a79e79
https://hg.mozilla.org/integration/autoland/rev/10817c138761
part 7 - Allow network request to proceed while annotating trackers - r=necko-reviewers,timhuang,jesup
https://github.com/mozilla-firefox/firefox/commit/6a591713e2fc
https://hg.mozilla.org/integration/autoland/rev/5929a4c1ff93
part 8 - Add telemetry and a Nimbus feature for anti-tracking channel classification - r=timhuang
https://github.com/mozilla-firefox/firefox/commit/5b07d408db9a
https://hg.mozilla.org/integration/autoland/rev/b429e06cdf71
part 9 - Add tests for deferred annotation - r=timhuang

Comment 24

[Norisz Fay [:noriszfay]]

https://hg.mozilla.org/mozilla-central/rev/f7c35b75ca12
https://hg.mozilla.org/mozilla-central/rev/9cc558447e22
https://hg.mozilla.org/mozilla-central/rev/287bc0c6367f
https://hg.mozilla.org/mozilla-central/rev/3eb414679ebd
https://hg.mozilla.org/mozilla-central/rev/9647c9aeda78
https://hg.mozilla.org/mozilla-central/rev/462e30702d71
https://hg.mozilla.org/mozilla-central/rev/10817c138761
https://hg.mozilla.org/mozilla-central/rev/5929a4c1ff93
https://hg.mozilla.org/mozilla-central/rev/b429e06cdf71

Comment 25

[Andrew Creskey [:acreskey]]

Ni' myself to see if I can get some early measurements of impact potential from this.

Comment 26

[Andrew Creskey [:acreskey]]

The newly-landed annotation deferral system is currently disabled in nightly: privacy.trackingprotection.defer_annotation.enabled

Looking at the probe that now tracks the entire classification time, glam: urlclassifier_check_channel_helper_time we can see that classification time is longer on Android, as expected.

Custom query: Fenix, urlclassifier_check_channel_helper_time time in ms

There is significant delay in later percentiles (ms)
e.g.

p_50    p_75   p_80   p_90   p_95   p_99
2.72	5.44   7.05   12.94  23.73  103.50

On desktop, the problem looks to be mostly at the tail
Custom query: Desktop, urlclassifier_check_channel_helper_time time in ms

p_50     p_75  p_80   p_90   p_95   p_99
0.96	1.05   1.36   3.23   7.69   47.45

I'll capture Fenix profiles with privacy.trackingprotection.defer_annotation.enabled enabled and disabled and share them here.

But we could consider simply enabling the feature, or else running a Nimbus experiment to measure impact.
Gecko pref-flip experiments on Android no longer require all the of additional plumbing of the past.

Comment 27

[Andrew Creskey [:acreskey]]

This Navigation PerfCompare adds a temporary PerfStat, mirroring the new glean probe check_channel_helper_time to see if we can detect a change from enabling privacy.trackingprotection.defer_annotation.enabled on our CI tests. (will take some days to resolve).

Comment 28

[Benjamin VanderSloot [:bvandersloot]]

I was working on some of this code for another bug and came to a realization: the check_channel_helper_time is capturing a fair bit of extra work.

https://searchfox.org/firefox-main/rev/e28b34ab33dbf49364999070168cbb7e11e8e5bd/netwerk/url-classifier/AsyncUrlChannelClassifier.cpp#1070-1085

The callbacks (incl. task->CompleteClassification(), which is the enabled path) are non-trivial.

Additionally we are capturing data on both the initial and deferred (non-blocking) classification.

I'm going to add a dependent bug to fix these issues on top of the bug I was working on (merge conflicts will abound). We probably will have some signal without this patch, but it definitely will be much cleaner after.

Comment 29

[Andrew Creskey [:acreskey]]

Thanks for the update, Benjamin. I'll keep an eye on bug 2043215.

In my current CI test where I enabled privacy.trackingprotection.defer_annotation.enabled I am generally seeing a huge regression in overall check_channel_helper_time (up to 90%!) -- maybe because of the time capturing issue?
Perf Compare

Related, this PerfStat for check_channel_helper_time might be worth landing
https://hg-edge.mozilla.org/try/rev/6981d626474d756dd31a8263c05d4ebf966827e4

Comment 30

[Benjamin VanderSloot [:bvandersloot]]

(up to 90%!)

Ouch! I don't think the fix to reduce noise would revert a performance regression if it is real.

The regression might be noise, but it also might be additional thread contention causing a performance degradation! I think we should revisit this once bug 2043215 lands. If there is still a degradation, we should pull out privacy.trackingprotection.defer_annotation.enabled and see if there is a different implementation approach that doesn't induce the thread contention.

Comment 31

[Andrew Creskey [:acreskey]]

A new PerfCompare showing the impact of enabling privacy.trackingprotection.defer_annotation.enabled on check_channel_helper_time in our Android CI pageload tests.

Jobs still running but looks very promising. Of course it's a new metric, so we will have to see.

Comment 32

[Andrew Creskey [:acreskey]]

Benjamin, the CI results of privacy.trackingprotection.defer_annotation.enabled look really good.

Picking two examples from the PerfCompare, (privacy.trackingprotection.defer_annotation.enabled as the new):

bing-search-restaurants perfstat-UrlClassifierCheckChannel, Android a55

From 67.91ms total channel classification time to 1.4ms
a 97.93 % Improvement

bild-de
From 266.38 ms total channel classification time to 104.28 ms
A -60.85 % Improvement of about 160ms.

The distributions look very solid [attached].

Comment 33

[Andrew Creskey [:acreskey]]

Attachment: bing-search-classification_time.png

Comment 34

[Andrew Creskey [:acreskey]]

Attachment: bild.de-classification time.png

Comment 35

[Andrew Creskey [:acreskey]]

If the privacy.trackingprotection.defer_annotation.enabled feature is ready, my suggestion would be to run a Nimbus pref-flip experiment on Android to better quantify field impact. As I was saying, these experiments no longer require extensive geckoview/fenix plumbing and I'm happy to help set it up and kick it off.
Alternatively, the pref could be landed enabled and we can try to observe impact (noting that nightly is very busy at the moment with happy eyeballs v3 changes landing).

Comment 36

[Benjamin VanderSloot [:bvandersloot]]

The feature is ready for experimentation in my view. I only wonder if these changes show up in less-specific metrics.

I think running an A/B experiment would be wise. And I would be interested to see what desktop does as well. But if Android is where you expect sensitivity, I'm here for just testing there. I'd just want to ship them together to keep the complexity down.

Comment 37

[Andrew Creskey [:acreskey]]

So far I would say that I'm seeing more signal of improvement on Android, and mostly on lower level timings like classification and overall network timings like HttpChannelAsyncOpenToTransactionPending.

However the PerfCompare did pickup significant improvements to document load time on two sites:

amazon loadtime
Android a55
1,161.56 ms > 1,111.14 ms
-4.822 % Improvement

bing-search-restaurants loadtime
Android a55
329.88 ms > 295.46 ms 
-3.943 % Improvement

I'll see if anything can observed in a local run.

Changes on MacOS look very modest (no Perfstats in this run, unfortunately)
PerfCompare

And we will have Windows numbers with Perfstats likely today.

(In reply to Benjamin VanderSloot [:bvandersloot] from comment #36)

The feature is ready for experimentation in my view. I only wonder if these changes show up in less-specific metrics.

I think running an A/B experiment would be wise. And I would be interested to see what desktop does as well. But if Android is where you expect sensitivity, I'm here for just testing there. I'd just want to ship them together to keep the complexity down.

Let me get some experiments ready then - we can do Desktop & Android simultaneously.

Comment 38

[Andrew Creskey [:acreskey]]

On a test of lives sites in Fenix, we see statistically significant improvements to the time spent blocking channels during classification:
Looking at the new Perfstat, UrlClassifierCheckChannel
(baseline → defer_annotation, median ms)

Site	Baseline	defer_annotation	Δ	Significant
bing obama	781	2	−99.7%	yes (p=0.0000)
bing restaurants	590	2	−99.7%	yes (p=0.0000)
ikea	420	7	−98.3%	yes (p=0.0000)
ikea outdoor	2563	233	−90.9%	yes (p=0.0000)
en.wikipedia Elvis	43	6	−86.0%	yes (p=0.0000)
wiki Mozilla	49	11	−77.6%	yes (p=0.0000)
cnn	47	11	−76.3%	yes (p=0.0000)
bild.de_	817	473	−42.2%	yes (p=0.0000)
bild royals article	369	408	+10.7%	no (p=0.47)

From the 9 urls that I checked, we picked up significant and reproduced improvements to a wikipedia page:
https://en.wikipedia.org/wiki/Elvis_Presley

en.wikipedia (Elvis)

Metric	Baseline	defer_annotation	Δ	p
SpeedIndex	1259	1237	−1.7%	0.0127
ContentfulSpeedIndex	886	856	−3.4%	0.0204
PerceptualSpeedIndex	1340	1298	−3.1%	0.0047
FirstVisualChange	848	799	−5.7%	0.0111
LastVisualChange	1724	1659	−3.8%	0.0138
VisualComplete85	1615	1566	−3.0%	0.0075
VisualComplete95	1704	1633	−4.2%	0.0048
FCP	826	772	−6.5%	0.0189
LCP renderTime	826	780	−5.6%	0.0304

All metrics reported here in google sheets.

Comment 39

[Andrew Creskey [:acreskey]]

I'll wait until the newly-added jobs complete before I summarize, but the Windows CI run has picked up improvements to URL classification delays as well as to higher order metrics user facing metrics including fcp and lcp.

Fenix and Desktop experiments are being put together, tentatively targeting a start date of Nightly, June 15th, 2026

Comment 40

[Andrew Creskey [:acreskey]]

Enabling privacy.trackingprotection.defer_annotation.enabled is showing improvements via our Windows CI navigation tests.
PerfCompare

UrlClassifierCheckChannel perfstat, URL classification delays is reduced:

Suite	Variant	Base→New (ms)	Δ
cnn	cold	47.4 → 2.1	−95.6%
instagram	cold	61.1 → 49.9	−18.3%
instagram	warm	49.1 → 40.9	−16.8%
amazon	cold	861.7 → 745.3	−13.5%

And we see broad cold-load user-facing improvements across sites

Suite	Metric	Base→New (ms)	Δ
cnn-nav.landing	FirstVisualChange	731.96 → 711.80	−4.63%
instagram	fcp	72.87 → 66.96	−4.62%
microsoft	largestContentfulPaint	348.39 → 342.15	−1.74%
paypal cold	fcp / largestContentfulPaint	185.07 → 182.20 ms	−2.19%
paypal warm	fcp / largestContentfulPaint	70.91 → 69.45 ms	−2.86%
facebook-nav.marketplace	SpeedIndex	386.99 → 385.39	−2.71%
facebook-nav.marketplace cold	largestContentfulPaint	530.91 → 521.32 ms	−1.98%
facebook-nav.marketplace	loadtime	563.83 → 549.07	−2.68%
facebook	fcp / fnbpaint	270.5 → 264.9	−1.13%

wikipedia cold showed mixed results, but given the broad improvements I don't think it's an issue.

Metric	Variant	Base→New	Δ (MWU)
HttpChannelCompletion (Network)	cold	29237 → 27398 ms	−7.7%
loadtime	cold	649.3 → 587.9 ms	−3.7%
fcp / fnbpaint	cold	320.9 → 333.8 ms	+4.5%
largestContentfulPaint	cold	331.8 → 343.9 ms	+5.0%

Comment 41

[Andrew Creskey [:acreskey]]

Nightly desktop and fenix experiments are underway.

This dashboard tracks the data as it arrives (sorry, requires mozilla SSO)

Benjamin, just to be sure, can you confirm that with privacy.trackingprotection.defer_annotation.enabled I should expect to see fewer submissions of the probe glean::urlclassifier::check_channel_helper_time?

In AntiTrackingChannelClassifierUtils::CheckChannelHelper. (To my understanding because we frequently skip the blocking task).

Comment 42

[Benjamin VanderSloot [:bvandersloot]]

Benjamin, just to be sure, can you confirm that with privacy.trackingprotection.defer_annotation.enabled I should expect to see fewer submissions of the probe glean::urlclassifier::check_channel_helper_time?

Yes, that would be expected. Since Bug 2043215's cleanup we don't measure the deferred annotation in the glean probes, so it we are only measuring the blocking urlclassifier queries. If there is no chance to block, we skip it.

Comment 43

[Andrew Creskey [:acreskey]]

Excellent, thank you.

So far I am seeing that the trend of improvements to url classification delays we observed in CI and in live site performance tests is playing out in both Firefox desktop and Fenix nightly populations via the experiment.

Dashboard - Defer URL Classification Annotation Experiment

Time spent waiting on URL classification, per client day, P75
Desktop: from ~84 seconds to ~40 seconds with deferred URL annotation
Fenix: ~27 seconds to ~13 seconds with deferred URL annotation
All data is preliminary

I think we would have to walk this experiment out to Release in order to look for broad improvements to higher order metrics like FCP and LCP. Happy to do so, as cloning and re-running experiments is very easy to do.

In the meantime, I think we should add a bug to enable this change by default since from everything I see we want to get this out to users as soon as possible.

Comment 44

[Benjamin VanderSloot [:bvandersloot]]

That's pretty remarkable, especially when you include the ~50% reduction in probes. P75 in the control is more fairly compared to P50 in the treatment.

Yeah, let's walk this out to release. We should also see if we can validate that our tracker annotation counts are not different between the branches before we pick a winner.

I'll file a bug blocked by this one to turn this on :)