Closed
Bug 617236
Opened 14 years ago
Closed 14 years ago
Investigate per tab memory usage increase caused by bug 558451 (Merge JSScope into JSScopeProperty, JSObject)
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
Tracking | Status | |
---|---|---|
blocking2.0 | --- | betaN+ |
People
(Reporter: emorley, Assigned: n.nethercote)
References
Details
(Keywords: regression, Whiteboard: [cib-memory][softblocker])
User-Agent: Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b8pre) Gecko/20101206 Firefox/4.0b8pre
Build Identifier:
In bug 598466 comment 116, it was ascertained that bug 558451, or more specifically, the following changeset:
http://hg.mozilla.org/tracemonkey/rev/e5958cd4a135
...caused an approximate 0.9MB/tab memory usage increase when following the STR in bug 598466 comment 13.
Reproducible: Always
Steps to Reproduce:
Using: Win7x64; win32 Minefield builds.
Setup:
1. Create a clean profile.
2. Open profile using nightly, disable all addons, turn off hardware acceleration & image discarding (to reduce other variables, but still occurs if they are on), restart nightly for changes to take effect.
3. Go to http://www.cad-comic.com/cad and middle mouse click on "random" 90 times to open a selection of tabs.
4. With prefs set to save tabs on exit, close nightly.
5. Make a copy of profile, so original not overwritten when testing each build.
Testing:
1. Open previously saved profile in nightly to be tested.
2. Ctrl+tab through all tabs to force them to load fully.
3. Wait 30 secs for GC/random processing to settle.
4. Measure private/working/virtual memory usage in MB using Sysinternals process explorer
Actual Results:
Memory usage using Tracemonkey branch: (private/working/virtual, in MB for 90 tabs)
2010-08-30--nightly: 787/812/974
2010-08-29--tryserver rev. e5958cd4a135: 777/802/958
2010-08-29--tryserver rev. eae8350841be: 697/723/887
2010-08-29--nightly: 691/715/875
~80MB increase for 90 tabs = ~0.9MB/tab
Caused by:
http://hg.mozilla.org/tracemonkey/rev/e5958cd4a135
Reporter | ||
Updated•14 years ago
|
Comment 1•14 years ago
|
||
I wonder if the root cause of this is bug 610070.
Comment 2•14 years ago
|
||
brendan, jorendorff, can you guys give us a first cut estimate of what's going on here? I'm thinking you might already have all the info we need. Basically, what I think we want to know is:
1. Was the memory increase entirely expected and intentional (~0.9MB per tab in the test setup of comment 0)? If yes, we are done here.
2. Is the unused space Julian discovered in bug 610070 intentional, or do we want to work to trim that?
Status: UNCONFIRMED → NEW
Ever confirmed: true
Reporter | ||
Comment 3•14 years ago
|
||
Using a different 68 tab profile (same STR otherwise) in the same bug (bug 598466 comment 94), the increase was 70-75MB, which equates to ~1.1MB/tab, so it would appear 0.9MB/tab is a fairly conservative/low-end estimate - if that helps at all.
Comment 4•14 years ago
|
||
The increase I expect from that patch is an additional 6 words or so per object that would previously have shared a scope. I would not have expected the number or size of any hash tables to increase.
Brendan, do you agree?
Comment 5•14 years ago
|
||
(In reply to comment #4)
> The increase I expect from that patch is an additional 6 words or so per object
> that would previously have shared a scope. I would not have expected the number
> or size of any hash tables to increase.
Could someone please check this using the test setup? I think the key data we want are (1) number of JSObjects live per page and (2) size comparison on the hash tables around this patch.
Assignee | ||
Comment 6•14 years ago
|
||
(In reply to comment #2)
>
> 2. Is the unused space Julian discovered in bug 610070 intentional, or do we
> want to work to trim that?
It looks unnecessary. See bug 610070 comment 8 for details.
Updated•14 years ago
|
Assignee: general → nnethercote
blocking2.0: ? → betaN+
Updated•14 years ago
|
Whiteboard: [cib-memory]
Comment 7•14 years ago
|
||
(In reply to comment #4)
> The increase I expect from that patch is an additional 6 words or so per object
> that would previously have shared a scope. I would not have expected the number
> or size of any hash tables to increase.
>
> Brendan, do you agree?
See bug 610070' about to be fixed.
Anything else is another bug to file.
/be
OS: Windows 7 → Windows XP
Assignee | ||
Comment 8•14 years ago
|
||
A summary for blocker drivers: hopefully bug 610070 will completely fix this memory increase. It's ready to land but blocked by another blocker. Once it lands, hopefully Ed will be able to re-measure.
Reporter | ||
Comment 9•14 years ago
|
||
Per comment 8, provisionally adding bug 610070 as dependency.
However, it's going to be hard to tell if bug 610070 landing has fixed the problem or not, unless someone takes the first known bad nightly and makes a try server build with that plus only the patch from bug 610070 and none of the other thousand changes in nightlies since (given that lots of other memory based changes/optimisations have occurred in that range).
Therefore, once the issue blocking bug 610070 is resolved, can someone take:
- mc rev e5958cd4a135 (first known bad)
- bug 610070 patch
- the blocker bug 619003 patch
...and make a opt win32 no tests tryserver build for me please, so I can tell for sure.
Thanks!
Depends on: 610070
Comment 10•14 years ago
|
||
Bug 619003 is fixed in tracemonkey.
/be
Updated•14 years ago
|
Whiteboard: [cib-memory] → [cib-memory][softblocker]
Assignee | ||
Comment 11•14 years ago
|
||
(In reply to comment #9)
>
> Therefore, once the issue blocking bug 610070 is resolved, can someone take:
> - mc rev e5958cd4a135 (first known bad)
> - bug 610070 patch
> - the blocker bug 619003 patch
> ...and make a opt win32 no tests tryserver build for me please, so I can tell
> for sure.
I tried to do this, but it's difficult. e5958cd4a135 is old enough that applying the patch from 610070 is painful.
Instead I've made you two builds, one without the bug 610070 patch, and one with that patch. (Bug 619003 was a blocker, but it just caused an obscure test failure, it shouldn't affect your testing.)
Here's the version without the patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/nnethercote@mozilla.com-fa032c74bf4c
Here's the version with the patch:
http://ftp.mozilla.org/pub/mozilla.org/firefox/tryserver-builds/nnethercote@mozilla.com-321bbd8333b6
Ed, can you compare them? I think you should be able to tell fairly accurately if we've got a ~0.9MB per-tab reduction. (Nb: at the time of writing they haven't yet been created, but tryserver tells me that's where they'll be once they finish building.)
Reporter | ||
Comment 12•14 years ago
|
||
Yeah I had a look at the patch and was thinking you might say that, given the number of inevitable changes that have been made in those areas since the older rev.
I'll give the tryserver builds a go once they are finished - and as you say, since they are identical apart from patch, any usage difference should be both fairly obvious and also attributable purely to bug 610070.
Thanks!
Reporter | ||
Comment 13•14 years ago
|
||
Using builds from comment 11...
Without patch: 906/926/1101 ; repeat 915/935/1110
With patch: 837/858/1034 ; repeat 843/864/1050
Therefore patch from bug 610070 has saved ~70MB across 90 tabs = ~0.78MB/tab, which isn't quite 0.9MB/tab, but difference due to measurement fluctuations perhaps?
Assignee | ||
Comment 14•14 years ago
|
||
Maybe measurement fluctuations, or maybe there's a small increase still present. But I think it's good enough to declare victory in this bug. Thanks for doing the measurements!
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•