Closed Bug 687935 Opened 13 years ago Closed 11 years ago

Tons of memory used by shapes (too many shapes live?) when this site is left open

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED WORKSFORME

People

(Reporter: bzbarsky, Unassigned)

References

()

Details

(Whiteboard: [Memshrink:P2])

Attachments

(1 file)

See url field.  When http://globoesporte.globo.com/futebol/times/gremio/ is left open for a while, memory usage continuously increases and most of it seems to be Shapes (though object slots are also pretty noticeable).

Why are we ending up with so many shapes, and can we avoid it?  This seems like echoes of the bugs where we cloned the shape table for dictionary shapes too much....
Blocks: 686307
Whiteboard: [Memshrink] → [Memshrink:P2]
Nick, I think you had looked into shape memory usage before, so handing this to you for now.
Assignee: general → nnethercote
After having the page open for an hour, I have this:

132,736,560 B (100.0%) -- explicit
├───81,923,527 B (61.72%) -- js
│   ├──25,407,707 B (19.14%) -- compartment(http://social.static.globo.com/pc/sc/index.html?eventoId=223&abas_inicial_logado=twitter&abas_inicial_naologado=comentaristas&abas_oficial=true&abas_twitter=true&abas_facebook=true&twitter_inicial=amigos&twitter_amigos=true&twitter_todos=true&facebook_inicial=amigos&facebook_amigos=true&facebook_todos=true&url=http%3A//globoesporte.globo.com/futebol/times/gremio/&cor_padrao=&css_customizacao=http%3A//s.glbimg.com/es/ge/o/futebol/times/gremio/desktop/azul-preto-times.css)
│   │  ├──15,110,144 B (11.38%) -- gc-heap
│   │  │  ├───7,914,432 B (05.96%) -- shapes
│   │  │  ├───4,169,744 B (03.14%) -- objects
│   │  │  ├───2,318,856 B (01.75%) -- arena-unused
│   │  │  ├─────281,152 B (00.21%) -- strings
│   │  │  ├─────127,872 B (00.10%) -- scripts
│   │  │  ├─────122,696 B (00.09%) -- arena-padding
│   │  │  ├─────118,048 B (00.09%) -- arena-headers
│   │  │  └──────57,344 B (00.04%) -- type-objects
│   │  ├───2,502,699 B (01.89%) -- analysis-temporary
│   │  ├───2,428,928 B (01.83%) -- mjit-code
│   │  │   ├──2,302,656 B (01.73%) -- method
│   │  │   ├─────70,112 B (00.05%) -- regexp
│   │  │   └─────56,160 B (00.04%) -- unused
│   │  ├───2,340,528 B (01.76%) -- object-slots
│   │  ├───2,168,544 B (01.63%) -- property-tables
│   │  ├─────286,336 B (00.22%) -- mjit-data
│   │  ├─────226,504 B (00.17%) -- type-inference
│   │  │     ├──109,696 B (00.08%) -- object-main
│   │  │     ├──106,824 B (00.08%) -- script-main
│   │  │     └────9,984 B (00.01%) -- tables
│   │  ├─────211,992 B (00.16%) -- script-data
│   │  ├──────84,960 B (00.06%) -- string-chars
│   │  ├──────41,504 B (00.03%) -- shape-kids
│   │  └───────5,568 B (00.00%) -- object-empty-shapes


7.9MB is quite a lots of shapes...
leave it open for a day and you see haw far it goes...
After leaving it open for 12 hours overnight, the big compartment looked like this:

267,088,176 B (100.0%) -- explicit
├──201,964,192 B (75.62%) -- js
│  ├──147,017,996 B (55.04%) -- compartment(http://social.static.globo.com/pc/sc
/index.html?eventoId=223&abas_inicial_logado=twitter&abas_inicial_naologado=come
ntaristas&abas_oficial=true&abas_twitter=true&abas_facebook=true&twitter_inicial
=amigos&twitter_amigos=true&twitter_todos=true&facebook_inicial=amigos&facebook_
amigos=true&facebook_todos=true&url=http%3A//globoesporte.globo.com/futebol/time
s/gremio/&cor_padrao=&css_customizacao=http%3A//s.glbimg.com/es/ge/o/futebol/tim
es/gremio/desktop/azul-preto-times.css)
│  │  ├──104,734,720 B (39.21%) -- gc-heap
│  │  │  ├───85,413,760 B (31.98%) -- shapes
│  │  │  ├───13,604,736 B (05.09%) -- objects
│  │  │  ├────2,824,992 B (01.06%) -- arena-unused
│  │  │  ├────1,089,504 B (00.41%) -- strings
│  │  │  ├──────818,240 B (00.31%) -- arena-headers
│  │  │  ├──────798,848 B (00.30%) -- arena-padding
│  │  │  ├──────127,296 B (00.05%) -- scripts
│  │  │  └───────57,344 B (00.02%) -- type-objects
│  │  ├───20,237,136 B (07.58%) -- object-slots
│  │  ├───16,783,712 B (06.28%) -- property-tables
│  │  ├────2,303,420 B (00.86%) -- analysis-temporary
│  │  ├────2,232,320 B (00.84%) -- mjit-code
│  │  │    ├──2,128,328 B (00.80%) -- method
│  │  │    ├─────63,776 B (00.02%) -- regexp
│  │  │    └─────40,216 B (00.02%) -- unused
│  │  ├──────225,680 B (00.08%) -- type-inference
│  │  │      ├──109,280 B (00.04%) -- object-main
│  │  │      ├──106,416 B (00.04%) -- script-main
│  │  │      └────9,984 B (00.00%) -- tables
│  │  ├──────211,848 B (00.08%) -- script-data
│  │  ├──────185,984 B (00.07%) -- mjit-data
│  │  ├───────56,104 B (00.02%) -- string-chars
│  │  ├───────41,504 B (00.02%) -- shape-kids
│  │  └────────5,568 B (00.00%) -- object-empty-shapes

85MB of shapes is, indeed, a lot.

W.r.t. comment 1:  we have two ways of storing shapes (see bug 631138 for details).  First, we store lots of them standalone on the JS heap, where they are linked into the property tree.  Then, certain ones (if searched for many times) can get copied into a hash table, called a property table.

In bug 610070 I changed the heuristic used to decide when to copy shapes into a property table, which greatly reduced the number of property tables created without hurting performance.

However, in this case the property tables aren't that big... well, 16.7MB is pretty big but compared to 85MB of vanilla shapes on the JS heap it doesn't seem that big.  So my experience from bug 610070 probably doesn't apply here, and my understanding of shapes (in particular, how you can end up with many of them) is limited.

bhackett, you know a lot about shapes -- any ideas?  Could TI be a factor?  I could try re-running it overnight with TI off if that would be useful.  Alternatively, I can add some debugging printf statements to the JS heap traversal done by about:memory if someone can suggest useful shape info to dump.
You can add a call to JS_DumpHeapComplete that I added in Bug 680482 to get the entire JS heap dumped to a file.
(In reply to Nicholas Nethercote [:njn] from comment #4)
> bhackett, you know a lot about shapes -- any ideas?  Could TI be a factor? 
> I could try re-running it overnight with TI off if that would be useful. 
> Alternatively, I can add some debugging printf statements to the JS heap
> traversal done by about:memory if someone can suggest useful shape info to
> dump.

TI shouldn't affect the shapes being created.  Given the number of shapes they are almost certainly going to primarily be dictionary shapes, which are specialized to a particular object --- each object in dictionary mode has a separate shape lineage.  You can check this by figuring out the fraction of traversed shapes which are inDictionary().

Assuming dictionary shapes are the issue, there are two possibilities I can see.  First is that there are a relatively small number of objects with a gigantic number of properties.  Not much can be done to fix this on the browser side.  Second is that there are a bunch of objects with similar shape lineages which *should* be using the common property table but are getting converted to dictionaries instead.  You should be able to get an idea of what's going on by js_DumpObject'ing all inDictionaryMode() objects found during heap traversal, and also by instrumenting the three places where toDictionaryMode() is called (objects growing over max height, reconfigure non-last property, remove non-last property).

Regardless of the cause, object shrinking will mitigate things by cutting shape memory by ~40% (on 32 bit platforms).  That doesn't help *that* much when leaving the site open though, since the site is pretty clearly leaking.  All object- and shape-related numbers went up a lot with the site left open, but script memory didn't budge.  How do other browsers do on this site?
IE 9 also leaks memory.
(In reply to Brian Hackett from comment #6)
> 
> Given the number of shapes
> they are almost certainly going to primarily be dictionary shapes, which are
> specialized to a particular object --- each object in dictionary mode has a
> separate shape lineage.  You can check this by figuring out the fraction of
> traversed shapes which are inDictionary().

Ah yes, I was mentally conflating dictionary mode shapes and shapes in property tables.  And I'd forgotten that I did some work on dictionary mode shapes in bug 630456.

Some mental refreshing for me, from bug 631138:

- The property tree height is bounded by MAX_HEIGHT.  Any object that gets
  more properties than that is effectively removed from the property tree
  and converted to "dictionary mode".  When this happens, a list of Shapes
  is created from the Shapes in the object's path, and a PropertyTable is
  also created for it.

- Dictionary-mode is triggered apart from MAX_HEIGHT by the delete operator
  or API equivalent removing a property from the tree. The original version
  of the code tried lazy tree forking (hg annotate if you are interested) but
  this still blows up for delete/add repetitions and the web finds every O(n^2)
  algorithm and makes its author pay.

- Dictionary mode is also triggered by changing attributes of a non-last property
  in an object of a given id -- the change attributes different values to the
  other members of the shape that contribute to its identity (but the parent says
  the same, so enumeration order does not change -- see bug 601399).

(bhackett said this more succinctly in comment 6: "objects growing over max height, reconfigure non-last property, remove non-last property".)

I also rediscovered bug 632653 -- big, slow arrays might bloat our shape count -- I wonder if that's relevant here.
(In reply to Nicholas Nethercote [:njn] from comment #8)
> I also rediscovered bug 632653 -- big, slow arrays might bloat our shape
> count -- I wonder if that's relevant here.

Good call, I forgot about these.  If slow arrays are a problem then things may be helped by bug 586842.
I did a 16 hour overnight run with added instrumentation.  First, in about:memory I distinguished dictionary mode objects and shapes:

434,252,216 B (100.0%) -- explicit
├──357,506,520 B (82.33%) -- js
│  ├──295,328,524 B (68.01%) -- compartment(http://social.static.globo.com/pc/sc/index.html?eventoId=223&abas_inicial_logado=twitter&abas_inicial_naologado=comentaristas&abas_oficial=true&abas_twitter=true&abas_facebook=true&twitter_inicial=amigos&twitter_amigos=true&twitter_todos=true&facebook_inicial=amigos&facebook_amigos=true&facebook_todos=true&url=http%3A//globoesporte.globo.com/futebol/times/gremio/&cor_padrao=&css_customizacao=http%3A//s.glbimg.com/es/ge/o/futebol/times/gremio/desktop/azul-preto-times.css)
│  │  ├──205,504,512 B (47.32%) -- gc-heap
│  │  │  ├──168,609,728 B (38.83%) -- shapes
│  │  │  │  ├──168,291,008 B (38.75%) -- dict
│  │  │  │  └──────318,720 B (00.07%) -- normal
│  │  │  ├───29,506,272 B (06.79%) -- objects
│  │  │  │   ├──29,396,472 B (06.77%) -- normal
│  │  │  │   └─────109,800 B (00.03%) -- dict
│  │  │  ├────2,032,760 B (00.47%) -- arena-unused
│  │  │  ├────1,953,312 B (00.45%) -- strings
│  │  │  ├────1,642,184 B (00.38%) -- arena-padding
│  │  │  ├────1,605,504 B (00.37%) -- arena-headers
│  │  │  ├──────150,528 B (00.03%) -- scripts
│  │  │  └────────4,224 B (00.00%) -- type-objects
│  │  ├───44,619,176 B (10.27%) -- object-slots
│  │  ├───40,755,008 B (09.39%) -- property-tables
│  │  ├────2,453,504 B (00.56%) -- mjit-code
│  │  │    ├──2,271,592 B (00.52%) -- method
│  │  │    ├────112,800 B (00.03%) -- regexp
│  │  │    └─────69,112 B (00.02%) -- unused
│  │  ├──────857,412 B (00.20%) -- analysis-temporary
│  │  ├──────631,904 B (00.15%) -- mjit-data
│  │  ├──────224,312 B (00.05%) -- string-chars
│  │  ├──────212,328 B (00.05%) -- script-data
│  │  ├───────48,384 B (00.01%) -- shape-kids
│  │  ├───────16,704 B (00.00%) -- type-inference
│  │  │       ├──12,480 B (00.00%) -- script-main
│  │  │       └───4,224 B (00.00%) -- object-main
│  │  └────────5,280 B (00.00%) -- object-empty-shapes

So, almost all shapes are dictionary mode, but very few objects are dictionary mode, which is interesting.

I also printed out a line at the three places where toDictionaryMode() was called, corresponding to the three cases in comment 8.  In each case I also printed out the number of dictionary shapes created by the conversion.  There were five cases that accounted for 99.1% of the lines (frequencies are on the left, info is on the right):

( 1)  31882 (61.0%, 61.0%): DICTIONARY MODE: removed the non-last property: 9
( 2)   8580 (16.4%, 77.4%): DICTIONARY MODE: removed the non-last property: 3
( 3)   5288 (10.1%, 87.5%): DICTIONARY MODE: exceeded MAX_HEIGHT: 129
( 4)   4241 ( 8.1%, 95.7%): DICTIONARY MODE: reconfigured the non-last property: 6
( 5)   1816 ( 3.5%, 99.1%): DICTIONARY MODE: removed the non-last property: 7

I'm not entirely sure what to make of this.  Given that object-slots is so large I'd guess we have a small number of very large objects.
(In reply to Nicholas Nethercote [:njn] from comment #10)
> I'm not entirely sure what to make of this.  Given that object-slots is so
> large I'd guess we have a small number of very large objects.

This looks right.  These objects may or may not be slow arrays, we should be able to tell which by checking the fraction of shapes that have a JSID_IS_INT property id.
It's looking good. I believe this bug which I've found will solve some important memory leak. It's a shame that I can't help on how to solve it cause I'm no programmer. But you are on the way I believe.
Well, the memory leak is almost certainly in the site (in that it creates more and more objects and holds on to them forever).  The only question is how we can minimize the damage (that is, let the site leak as little memory as possible).

njn, can we just dump a JS stack on some of those transitions to dictionary mode where we're removing the non-last property?  Are the 9, 3, etc heights or property names?
(In reply to Boris Zbarsky (:bz) from comment #13)
> Well, the memory leak is almost certainly in the site (in that it creates
> more and more objects and holds on to them forever).  The only question is
> how we can minimize the damage (that is, let the site leak as little memory
> as possible).

Yes, the fact that it leaks isn't the issue, it's that our shape usage is excessive.

> njn, can we just dump a JS stack on some of those transitions to dictionary
> mode where we're removing the non-last property?  Are the 9, 3, etc heights
> or property names?

The 9, 3, etc are the number of properties in the object when it's converted to dictionary mode.

I added the JSID_IS_INT instrumentation like bhackett suggested, after running for an hour I see this:

│   │  │   ├──4,558,784 B (02.74%) -- shapes
│   │  │   │  ├──4,185,792 B (02.51%) -- dict-int
│   │  │   │  ├────300,736 B (00.18%) -- normal
│   │  │   │  └─────72,256 B (00.04%) -- dict

So it sure looks like big slow arrays are the problem.  I'm guessing the 9/3/6/7 cases are irrelevant.
Depends on: 586842
so?
Hopefully 586842 will fix this.  Waldo is working on it, but it's a big change and will take a while.
No longer blocks: 686307
(In reply to Nicholas Nethercote [:njn] from comment #16)
> Hopefully 586842 will fix this.  Waldo is working on it, but it's a big
> change and will take a while.

There's no update in that bug since 07/2011, are they working on it?
> There's no update in that bug since 07/2011, are they working on it?

Yes.  Look at the list of "depends on" bugs.  Four of these have been fixed and the other two have patches.  There may be other dependent bugs added.  As I said, it's a big change and will take a while, but I don't know exactly how long.
Assignee: n.nethercote → general
Attached image Another example
Just as happened when I first uncovered this bug (https://bugzilla.mozilla.org/show_bug.cgi?id=686307) I have found that other websites produces some similar flaws, as show in the image. The curious thing is that 86.34 MB (04.99%) -- compartment(http://patriciaveigapf.blogspot.com.br/) is a closed tab, which remains using memory. Also that facebook compartment is a closed tab, there's no facebook page open whatsoever. And the garbage collection does nothing to free that memory. Only in I close and reopen firefox they get cleaned. Something is wrong here.
Months and months later this bug is still happening exactly as before. Anyone can check lefting open http://globoesporte.globo.com/futebol/times/gremio/ I believe someone should fix this, because the same leakage also happens with other websites, with unlimited amounts of memory being lost.
Waldo's been making steady progress on the blocking bug 586842, but it's a big change.  He recently got back from 5 weeks of vacation so hopefully he'll make some more progress soon.
I can no longer reproduce this.  Bug 827490 may have helped, or the page may have changed.  I no longer see compartments with URLs that begin with http://social.static.globo.com/...

fscussel, can you still reproduce this problem in a Nightly build?
I will try.
I will close this, based on comment 23 and the lack of follow-up.
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → WORKSFORME
You need to log in before you can comment on or make changes to this bug.