Closed Bug 1440747 Opened 3 years ago Closed 3 years ago

Don't load onboarding frame script into every frameloader

Categories

(Firefox :: Tours, enhancement, P1)

enhancement

Tracking

()

RESOLVED FIXED
Firefox 61
Tracking Status
firefox61 --- fixed

People

(Reporter: kmag, Assigned: kmag)

References

Details

(Whiteboard: [MemShrink:P2])

Attachments

(1 file)

We currently wind up wasting a huge amount of memory by loading the onboarding.js framescript into every frameloader of every content process, even though we only actually need it when we're loading about:home or about:newtab.
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #0)
> We currently wind up wasting a huge amount of memory by loading the
> onboarding.js framescript into every frameloader of every content process,

Out of curiosity, how did you establish this (and how much is a "huge amount")? I recently tried to use about:memory to figure out which (if any) frame scripts were using a lot of memory, and couldn't see any way of getting that information, but I'm presumably missing something.
Flags: needinfo?(kmaglione+bmo)
(In reply to :Gijs from comment #2)
> Out of curiosity, how did you establish this (and how much is a "huge
> amount")?

Not easily. We currently have somewhere over 1MB of string data in every empty content process, according to about:memory. Most of that is "non-notable" strings, so I lowered the threshold for notable strings to figure out what most of it was, and then tracked those strings down to scripts.

It's hard to say exactly how much this change saves, but it seems to be somewhere on the order of 10s of KB to 100KB per frame script. I stopped looking in detail once it was clear that this made a measurable difference.
Flags: needinfo?(kmaglione+bmo)
Priority: -- → P1
Attachment #8953564 - Flags: review?(dtownsend) → review?(gijskruitbosch+bugs)
Comment on attachment 8953564 [details]
Bug 1440747: Don't load onboarding frame script into every frameloader.

https://reviewboard.mozilla.org/r/222782/#review229590

Stealing because I looked at this already. r=me
Attachment #8953564 - Flags: review?(gijskruitbosch+bugs) → review+
(In reply to :Gijs from comment #4)
> Stealing because I looked at this already. r=me

Thanks
(In reply to Kris Maglione [:kmag] (long backlog; ping on IRC if you're blocked) from comment #5)
> (In reply to :Gijs from comment #4)
> > Stealing because I looked at this already. r=me
> 
> Thanks

Do you plan on landing this?
Flags: needinfo?(kmaglione+bmo)
Whiteboard: [MemShrink] → [MemShrink:P2]
Yeah, sorry, conflicts landed almost as soon as I wrote this, and I got side-tracked before I had a chance to resolve them
Flags: needinfo?(kmaglione+bmo)
https://hg.mozilla.org/integration/mozilla-inbound/rev/8fbe9eda393abf440bf773ff6512a17fa6daad23
Bug 1440747: Don't load onboarding frame script into every frameloader. r=Gijs
https://hg.mozilla.org/mozilla-central/rev/8fbe9eda393a
Status: NEW → RESOLVED
Closed: 3 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 61
You need to log in before you can comment on or make changes to this bug.