926713 - AWSY: ~4.07 MiB regression in startup resident memory from bug 717490

Status: RESOLVED WONTFIX

(Firefox for Android Graveyard :: General, defect)

Description

[Kartikaya Gupta (email:[e-mail])]

Attachment: Diff of about:memory dumps

http://areweslimyet.com/mobile shows a regression in startup resident memory (top graph, bottom line) in this range of inbound changesets:

502404f03662019df1f77342220b4b38aa432c2d 127.51MiB
655490859d7780e5821cbce06618d768bbd24b30 131.58MiB Δ 4.07MiB

That second changeset is a merge from m-c which includes a bunch of changes. However, I'm now also running the AWSY harness on fx-team builds, which allows me to pinpoint the regression:

fx-team rev  : startup resident memory
--------------------------------------
da9f409953d5 : 133414912
daa8fbedef2f : 133361664
a141e39bf6da : 133840896
c96a2d66725c : 133378048
905feca3578b : 133275648
5055f99ce7c9 : 133267456
00955d61cc94 : 137596928 <-- regression!
cd6752c496fd : 137940992
6e889bf97a59 : 138035200
c7cd2a9b50a7 : 136630272
4888e018458d : 137973760
dc3d80fd48b0 : 138301440

The regression range is https://hg.mozilla.org/integration/fx-team/pushloghtml?fromchange=5055f99ce7c9&tochange=00955d61cc94 which is just one changeset, from bug 717490.

The diff of the about:memory dump before after the change is attached; unfortunately it doesn't really provide much insight into where the extra memory is going. I think recent changes to the about:memory dump removed some smaps info that in retrospect might have provided more information.

Comment 1

[Nicholas Nethercote [inactive]]

dolske, any idea what might have happened here?

Comment 2

[Justin Dolske [:Dolske]]

The memory diff lists crypto-SDR.js, which is what I'd expect from bug 717490 comment 17. It's just non-lazily instantiating a component that would have been (lazily) instantiated later, anyway. I'm a bit surprised that's 4MB, since it's a pretty lightweight component (what's the baseline cost these days?).

The rest of that patch is code removal, so I'd not be suspecting anything else.

A simple(?) check would be to comment out this line:

http://mxr.mozilla.org/mozilla-central/source/toolkit/components/passwordmgr/storage-mozStorage.js#192

192         try {
193             // Force initialization of the crypto module.
194             // See bug 717490 comment 17.
195             this._crypto;
196

Comment 3

[Kartikaya Gupta (email:[e-mail])]

Indeed you are correct. I have a baseline try push at [1] and one with the above modification at [2]. The data from my AWSY runs on that shows that the "Start" resident memory usage drops from 138162176 to 133957632 which fixes the regression.

Considering that the "StartSettled" datapoints of the two builds are the same (meaning that even with the lazy instantiation, the code is loaded within 30 seconds of startup anyway) I'm not inclined to push particularly hard on this regression.

There does also appear to be a small startup time improvement (2332.80ms -> 2296.60ms, as measured by the difference between "chrome startup finished" and "throbber stop" zerdatime output) associated with making the instantiation lazy. Not sure if that's enough to warrant looking into this further. mfinkle?

[1] https://tbpl.mozilla.org/?tree=Try&rev=b8399b63cba2
[2] https://tbpl.mozilla.org/?tree=Try&rev=deca8b016ee1

Comment 4

[Mark Finkle (:mfinkle) (use needinfo?)]

We can't make the crypto call go away, or even move it around too much. The only play I'd be up for is digging into why the call has a high cost to begin with and that's not in the scope of this bug.

I think we let this go.