618031 - Investigate per tab memory usage increase caused by bug 462076 (Dynamically inserted iframes on refresh sometimes trade places)

Status: RESOLVED WONTFIX

(Core :: General, defect)

Description

[Sheila Mooney]

As part of the analysis in bug 598466, there was an identified increase B of about 50MB. This bug is to determine if the increase was expected/intentional and if there are any ideas for mitigating this.

Comment 1

[Joe Drew (not getting mail)]

This might be intentional. As Boris says in bug 598466 comment 65, we need to see if it persists if there are no images in the tabs in question.

(There have also been improvements with active tabs recently, in that prior to the bug in question, which I can't find right now, most tabs were marked as active forever iirc.)

Ed, can you help us out (or point to where you have already helped us out)?

Blocking final for now so we make sure we look into it, but this might just end up being closed wontfix.

Comment 2

[Ed Morley [:emorley]]

Thanks for breaking this out and ensuring it got followed up, I was going to go
through bug 598466 and follow up the loose ends at some point, just hadn't had
a chance.

Is the bug Boris refers to (bug 512260) related to images being discarded iff image.discardable=true? If so, then how can it affect the result, since I was testing with discardable=false? 

Also, the follow-up fix for it, bug 615194 (which is what I believe you where referring to as "improvements with active tabs recently") should (a) have only been an issue when discarding=true, and (b) was avoided in the original STR, since it involved cycling through all tabs using ctrl+tab, thereby making each as non-active anyway.

However, I will retest using tabs with no images (as per suggestion in bug 598466 comment 65) and post back the results shortly. The only problem is that I presume using about:blank is going to effect the results in multiple ways, given that about:blank presumably doesn't contain any JS etc etc and so might hide any memory increase anyway?

Thanks!

Comment 3

[Ed Morley [:emorley]]

I've retested the increase B range using 100 tabs of about:licence (since it actually has some content, unlike about:blank (but doesn't have pictures either) - with the following results:

2010-08-17: 291/281/531
2010-08-18: 287/275/540

However, this doesn't mean that things like javascript memory usage increases can't have been responsible instead of images.

Also, what about the questions raised in paragraphs 2 and 3 of comment 2?

Thanks!

Comment 4

[Ed Morley [:emorley]]

For reference... (from bug 598466 comment 61)

#Increase B (of ~50MB for 68 tabs)
Last good nightly: 2010-08-17 
First bad nightly: 2010-08-18
Pushlog:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=9ef027bf2120

How about some tryserver builds to narrow the range a bit more?

Comment 5

[Timothy Nikkel (:tnikkel)]

We're not sure it's bug 512260?

Anyway, try builds will appear in a few hours at

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-5b2a34b01e20
m-c e486018949e2

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-15415932e1cc
m-c 9594f23591a1

(these are just the changesets before and after that bug)

Comment 6

[Ed Morley [:emorley]]

The try builds are now in /old/ but have finally downloaded them and tested using the 90 tab testcase profile.

2010-08-17 -- 9594f23591a1: 722/971/1325
2010-08-17 -- e486018949e2: 728/975/1329

Therefore it would appear that bug 512260 is not the cause after all, changing title to reflect this.

Could someone kindly make some more win32 opt no tests tryserver builds in this range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=e486018949e2

Thanks!

Comment 7

[Timothy Nikkel (:tnikkel)]

Builds will be available in a few hours at

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-3a24ef7fb129
(m-c 55953a91b4d6)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-7cce516167e0
(m-c 40fa4ebeacbb)

Comment 8

[Ed Morley [:emorley]]

Thank you for the try builds/sorry for the delay, will be a quicker turnaround now that I'm back after Christmas.

Results using 90 tab testcase:
2010-08-17 -- 55953a91b4d6: 673/695/855
2010-08-17 -- 40fa4ebeacbb: 678/701/854
...therefore both before memory increase.

New range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=55953a91b4d6&tochange=e486018949e2

Comment 9

[Timothy Nikkel (:tnikkel)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-a5d946ad4b32
(m-c 984f55359541)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-13c14119db8d
(m-c a27043cd19ff)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-82aa65eeaaa1
(m-c ca457b5758e0)

Comment 10

[Ed Morley [:emorley]]

2010-08-17 -- ca457b5758e0: 685/710/874
2010-08-17 -- a27043cd19ff: 672/696/855
2010-08-17 -- 984f55359541: 681/702/865
...so all before the memory increase.

Reduced range...
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=ca457b5758e0&tochange=e486018949e2

Almost there! :-)

Comment 11

[Ed Morley [:emorley]]

Ok, slight confession to make.

Since the latest reduced range contained only audio API changes and omnijar backouts (which didn't seem likely suspects), I retested the try builds used in comment 8 and comment 10 as well as the 17th and 18th nightlies to verify the results. It would appear that the memory usage figure I was comparing against (comment 6, ~720MB) was using a slightly different profile - so I was wrong to say they were before the increase (they were in fact after). Apologies for the mixup, I was having to test away from home using my laptop, so didn't have all the figures in front of me.

Have double-checked all results (making sure that the correct profile was used this time) and combined them in date descending order...

2010-08-18 nightly (9ef027bf2120): 683/707/862
2010-08-17 -- 9594f23591a1: 684/705/863
2010-08-17 -- e486018949e2: 688/646/864
2010-08-17 -- ca457b5758e0: 685/710/874
2010-08-17 -- a27043cd19ff: 672/696/855
2010-08-17 -- 984f55359541: 681/702/865
2010-08-17 -- 55953a91b4d6: 673/695/855
2010-08-17 -- 40fa4ebeacbb: 688/656/867
2010-08-17 nightly (116f2046b9ef): 628/654/802

Therefore the new/correct reduced range... 
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=40fa4ebeacbb

NB: The conclusion that bug 512260 is not in any way related, is still correct.

Anyway, sorry for the confusion/misdirection!

Comment 12

[Timothy Nikkel (:tnikkel)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-d71b60faf882
(m-c 2dbb1278a15c)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-5906322810a6
(m-c 007d994cac53)

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-63bb7e3f2e65
(m-c 353da09ea0dd)

Comment 13

[Ed Morley [:emorley]]

Thanks for the builds again :-)

Slight duplication, but hopefully makes fluctuations vs actual increases clearer... 
(date descending order again)
2010-08-18 nightly (9ef027bf2120): 683/707/862
2010-08-17 -- 9594f23591a1: 684/705/863
2010-08-17 -- e486018949e2: 688/646/864
2010-08-17 -- ca457b5758e0: 685/710/874
2010-08-17 -- a27043cd19ff: 672/696/855
2010-08-17 -- 984f55359541: 681/702/865
2010-08-17 -- 55953a91b4d6: 673/695/855
2010-08-17 -- 40fa4ebeacbb: 688/656/867
2010-08-17 -- 2dbb1278a15c: 691/715/873 <- Try build from comment 12
2010-08-17 -- 007d994cac53: 683/708/863 <- Try build from comment 12
2010-08-17 -- 353da09ea0dd: 690/716/864 <- Try build from comment 12
2010-08-17 nightly (116f2046b9ef): 628/654/802

Reduced range:
http://hg.mozilla.org/mozilla-central/pushloghtml?fromchange=116f2046b9ef&tochange=353da09ea0dd

Comment 14

[Timothy Nikkel (:tnikkel)]

http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/[e-mail]-8f46c99c990a
(m-c f43f9b764efb)

Comment 15

[Ed Morley [:emorley]]

2010-08-17 -- 353da09ea0dd: 690/716/864 (repeat: 678/703/856)
2010-08-17 -- f43f9b764efb: 616/642/793 (repeat: 621/646/795)

Therefore following changeset is cause:
http://hg.mozilla.org/mozilla-central/rev/353da09ea0dd
Bug 462076 - Dynamically inserted iframes on refresh sometimes trade places

CCing Olli.

Summary for Olli: 
Using the STR in bug 598466 comment 13, a 90 tab profile was created of pages from http://www.cad-comic.com/cad and a ~60-70MB overall increase in private memory usage (ie ~0.7MB/tab) found after bug 462076 landed. The results given above in the format zzz/zzz/zzz are private/working/virtual in MB. Hardware acceleration was disabled as well as all plugins.

Comment 16

[Olli Pettay [:smaug][[e-mail]]]

I'll try to reproduce.

Bug 462076 did change bfcache handling. In some cases we can keep more
things in memory, in some cases we keep less.

Comment 17

[Olli Pettay [:smaug][[e-mail]]]

I *may* have reproduced the problem on linux.

Bug 462076 does not affect to the number of DOMWindows or nsDocShells.
It shouldn't affect to the number of nsDocument objects either.
Some more nsSHEntry objects are created, but those are rather small, and there
aren't too many of them anyway.

One thing which may affect mem numbers quite a bit is that Bug 462076
fixed Bug 307241. Before bug 462076 loading the testcase page did cause
the assertion/warning and that also means that some (i)frame's 
nsSHEntry was replacing existing nsSHEntry in the session history
and that old entry might have contained a content viewer (which keeps the whole
layout object tree alive). If that is the case, in the new setup we may
have more live content viewer objects at a time.
Investigating some more tomorrow...

Comment 18

[Olli Pettay [:smaug][[e-mail]]]

After loading all the documents we end up exactly the same number
of content viewers.

I need to still verify what happens to documents.

Comment 19

[Olli Pettay [:smaug][[e-mail]]]

And after all the pages have been loaded, there are exactly same number
documents alive before and after bug 462076.

There is some difference though. After bug 462076 some more content viewers and
documents are created, and then deleted. So seems like we temporarily keep
some more objects alive and then release them.

Comment 20

[Olli Pettay [:smaug][[e-mail]]]

Massif hasn't given me any useful information.

Comment 21

[Olli Pettay [:smaug][[e-mail]]]

And there same number nsSHEntry objects alive...

Getting hard to see what could be improved.

Comment 22

[Mike Beltzner [:beltzner, not reading bugmail]]

** PRODUCT DRIVERS PLEASE NOTE **

This bug is one of 7 automatically changed from blocking2.0:final+ to blocking2.0:.x during the endgame of Firefox 4 for the following reasons:

 - it was marked as a soft blocking issue without a requirement for beta coverage

Comment 23

[Nicholas Nethercote [inactive]]

smaug, what's the status of this?  Is it worth keeping open?

Comment 24

[Olli Pettay [:smaug][[e-mail]]]

So far comment 19 and comment 21 are all the information I have about this.
Hard to see what could be improved.

Comment 25

[Nicholas Nethercote [inactive]]

(In reply to comment #24)
>
> Hard to see what could be improved.

Should we resolve this WONTFIX then?