1827167 - 1.03 - 1.02% Base Content JS / Base Content JS + 2 more (Linux, OSX, Windows) regression on Mon April 3 2023

Status: NEW

(Firefox :: Search, defect, P2)

Description

[Acasandrei Beatrice (needinfo me)]

Perfherder has detected a awsy performance regression from push 3bfe136cc0e54c1910c085c362f650f4c60ac185. As author of one of the patches included in that push, we need your help to address this regression.

Regressions:

Ratio	Test	Platform	Options	Absolute values (old vs new)
1%	Base Content JS	linux1804-64-shippable-qr	fission	1,658,427.33 -> 1,675,564.67
1%	Base Content JS	linux1804-64-shippable-qr	fission	1,658,425.67 -> 1,675,560.00
1%	Base Content JS	macosx1015-64-shippable-qr	fission	1,658,836.00 -> 1,675,734.67
1%	Base Content JS	windows11-64-2009-shippable-qr	fission	1,660,835.33 -> 1,677,813.33

Details of the alert can be found in the alert summary, including links to graphs and comparisons for each of the affected tests. Please follow our guide to handling regression bugs and let us know your plans within 3 business days, or the offending patch(es) may be backed out in accordance with our regression policy.

If you need the profiling jobs you can trigger them yourself from treeherder job view or ask a sheriff to do that for you.

For more information on performance sheriffing please see our FAQ.

Comment 1

[James Teow [:jteow]]

Setting this to S3, P2 for now as 1% is well below the 10% threshold. Will talk to a Platform Engineer to find out more on what realistic options we have.

Comment 2

[Andrew McCreight [:mccr8]]

At jteow's request, I looked at this a bit. The actual regression is rather small, around 17,000 bytes. If this really needed to be loaded in every content process, I'd say that this isn't a big deal, as this is probably just some data structure churn. Here's the about:memory diff of some JS related memory:

├───0.13 MB (-04.40%) -- js-non-window
│ ├──0.08 MB (-02.54%) ++ zones/zone(0xNNN)
│ └──0.06 MB (-01.87%) -- runtime
│ ├──0.03 MB (-01.04%) ── shared-immutable-strings-cache [8]
│ └──0.03 MB (-00.83%) ── script-data [8]

However, it sounds this actor only cares about some search related pages based on their domain (well, ultimately it wants the entire URL, but I think it can filter based on the domain at first), so we shouldn't be loading it in every child process. This test is loading blank pages from some trivial domain, so the overhead should be zero.

Nika, is there some way to decide in the parent process whether to load an actor at all based on the domain? Thanks.

Comment 3

[Nika Layzell [:nika] (ni? for response)]

(In reply to Andrew McCreight [:mccr8] from comment #2)

Nika, is there some way to decide in the parent process whether to load an actor at all based on the domain? Thanks.

Yes, you can add the limitation with webextension-style matchpatterns (https://searchfox.org/mozilla-central/rev/504bd0a7ee3110ecb2b31f89b9ce54faf76e228b/dom/chrome-webidl/JSWindowActor.webidl#97-104). You can see it used for about URIs here (https://searchfox.org/mozilla-central/rev/504bd0a7ee3110ecb2b31f89b9ce54faf76e228b/browser/components/BrowserGlue.sys.mjs#468), though it will also work for other webextension patterns, so you can match based on domains with that.

Comment 4

[BugBot [:suhaib / :marco/ :calixte]]

Set release status flags based on info from the regressing bug 1816729

Comment 5

[James Teow [:jteow]]

Thanks for the help :nika and :mccr8.

After discussing with :standard8, short term we'll try to prevent the consistent regressions of these tests by adding a match for urls starting with https.

I've filed follow-up bug 1827536 where we'll investigate dynamically adding a list of match expressions from search-telemetry-v2 that is more specific to the SERP URL's. This is so that if a match has to be changed on the fly, it can be done with remote settings rather than directly manually updating BrowserGlue.

Comment 6

[Ryan VanderMeulen [:RyanVM]]

James, Fx113 goes to RC in a bit over a week. Is the short-term fix mentioned in comment 5 on the radar for this cycle?

Comment 7

[James Teow [:jteow]]

Hi Ryan, Bug 1824897 is very similar (if not practically the same) as this, as its same child actor. We issued a short-term fix of limiting to only load on pages with https which passes the AWSY tests but doesn't fix the underlying problem.

We opted to keep this open because the actual fix that would benefit users most of the time is Bug 1827536.