[patch][ccla]possible gfx memory leaks when many shapes are created

[Patrick Ruzand]

When a huge number of shapes are created (for ex, a monitoring application that creates/destroy many shapes at regular intervals), the IE allocated memory keeps increasing.

The attached sample reproduces the issue. Run it for a long time (1-2h) and monitor the browser memory using a task mgr.

Happens in IE9 (all doc mode) or IE8. This is NOT reproducible on FF or Safari.

[Patrick Ruzand]

testcase demonstrating the issue.

[Patrick Ruzand]

The problem appears because of the gfx.shape.register function that creates new ids (strings) for each shape.
When the number of shapes that are created is huge (and so the number of id strings), IE seems not to be able to collect all the intern strings, even if there are no references to ids anymore.

[Patrick Ruzand]

fix images path

[Patrick Ruzand]

What I have found:

• I thought it was due to the getUID closure that could not be GCed due to the ref to the id string. It's not: extracting the getUID to be a Shape method with a _uid field that is nullified when the shape is detroyed has no effect. That is:

instead of

constructor: function(){
  ...
  var uid = shape.register(this);
  this.getUID = function(){
    return uid;
  }
}

replaced by:

constructor: function(){
  ...
  this._uid = shape.register(this);
},
getUID: function(){
  return this._uid;
},
destroy: function(){
  shape.dispose(this);
  this._uid = null;
}

• it's not a leak due to the registry map. Even if I comment the call to registry[uid] = shape, the problem still there.

• The fact that FF or Safari memory is flat (for Safari, it increases then stabilizes) leads me to think the gfx api implementation is handling the cleanup correctly.

[Patrick Ruzand]

A possible patch would be to recycle the ids of the destroyed shapes. This way, it would limit the number of intern strings the browser has to handle.
But since recycling ids makes an id not "unique" anymore, and therefore may have some possible impacts on existing application, and because this use case where thousands of shapes are created and destroyed during the app lifecycle is quite specific and due to IE bad memory management, the patch would need to be enabled via a dojoConfig flag.

I attach such patch to the ticket. The idea is:

• it is enabled via the recycleGfxIds dojoConfig property.
• when disabled, the existing behavior is kept.
• when enabled, the id of a destroyed shape is stored in a cache, and when a new shape is registered, the cache is checked for available ids.

[Patrick Ruzand]

patch by pruzand (IBM,CCLA)

[ben hockey]

is there some other way to achieve this? it seems that using the type of shape as part of the key into the registry is never leveraged. do you see improvements if you just share a single number as the id for all shapes and only use that number as the index into the registry?

var uid = 0,
    registry = {};
shape.register = function (s) {
    registry[++uid] = s;
    return uid;
};

this has the following advantages:

• does not rely on declaredClass. you can't rely on declaredClass to exist in any shape subclasses so it would be good not to use it if possible.
• might help memory consumption by not creating extra unused strings with s.declaredClass.split('.')
• avoids any string concatenation in order to produce the key into the registry - it's always just the next number in sequence.

if for some reason you really need to rely on declaredClass then maybe you could try using the full declaredClass as the type rather than just the last segment - this may use less memory compared to splitting it to get just the last segment but it will still involve a string concatenation in order to add a unique number for that type.

if you are planning to leverage the type in the future then you'll need a different approach than what you're using currently. you have a single hash where the keys are going to be something like Rect0, Rect1, Line0, Line1, Line2. if you want to leverage the type to retrieve shapes from the registry, you're still going to need to iterate all the keys of the hash and compare them against the type you're looking for. a better approach would be a separate hash for each type so that registry ends up looking something more like:

{
    Rect: {
        0: ...,
        1: ...
    },
    Line: {
        0: ...,
        1: ...,
        2: ...
    }
}

i'd rather try using just a single id and ignoring the type rather than add more complexity such as a recycleGfxIds config property. i'd like to only use something like that as a last resort. i don't think we've explored all the options yet.

[Patrick Ruzand]

Replying to neonstalwart:

is there some other way to achieve this? it seems that using the type of shape as part of the key into the registry is never leveraged.

yes, the id could in fact be whatever. The current implementation had the benefit of telling something about the shape, but it could be anything in theory.

do you see improvements if you just share a single number as the id for all shapes and only use that number as the index into the registry?

I tried that for the very same reasons you mentioned. Honnestly, I don't remember exactly as I was trying to isolate the issue (so in the middle of lots of tests), but IIRC, while the increase was much smaller, it was still increasing. But as I said, I don't remember exactly (IE9 or IE8, different doc mode), so I will do another pass to be sure. I will post the results here.

if for some reason you really need to rely on declaredClass then maybe you could try using the full declaredClass as the type rather than just the last segment - this may use less memory compared to splitting it to get just the last segment but it will still involve a string concatenation in order to add a unique number for that type.

There were no particular reasons for the declaredClass other than it was informative about the shape. So in theory, we could change the id to be simple numbers. My only concern is about the existing version (1.7 and 1.8). While the id format has not be documented (so users are not supposed to expect a specific format), changing it in a patch could maybe break an app that was expecting a particular id format ? (But I doubt).

i'd rather try using just a single id and ignoring the type rather than add more complexity such as a recycleGfxIds config property. i'd like to only use something like that as a last resort.

I totally agree, I would really prefer something else than this flag patch, and this should indeed be the last resort. That's why all your feedbacks is more than welcome !

[Patrick Ruzand]

Replying to pruzand:

do you see improvements if you just share a single number as the id for all shapes and only use that number as the index into the registry?

I tried that for the very same reasons you mentioned. Honnestly, I don't remember exactly as I was trying to isolate the issue (so in the middle of lots of tests), but IIRC, while the increase was much smaller, it was still increasing. But as I said, I don't remember exactly (IE9 or IE8, different doc mode), so I will do another pass to be sure. I will post the results here.

So I did some tests, and it confirms indeed that the problem is still there on IE8. Indeed, under IE8, the memory increased (but at much slower pace) without stabilizing.
Under IE9/IE9Standard, the memory first increased then stabilized.

[Patrick Ruzand]

some numbers for the "simple counter" impl:

IE9 Browser Mode/IE9Standard document mode:

start time	Mem.
9:53	46732K
12:57	85294K

IE8 Browser Mode/ IE8Standard document mode:

start time	Mem.
6:53	45520K
9:51	124264K

(count = 1,258,788)

[Patrick Ruzand]

With the alpha build coming tomorrow (Feb, 20th), I would like to have the patch included in the alpha to benefit from the testing period. Any objections ? (or an alternative patch ? )

[ben hockey]

i'm really cautious about applying a patch without understanding the underlying reason for why the memory is leaking in the first place. it increases the public API by introducing a flag that needs to be kept for backwards compatibility even if we find out that the cause of the issue was not the strings but some other side effect. perhaps there is something in your new patch that makes it possible to do some gc that wasn't possible with just deleting the property from the registry - you are doing more than just recycling ids.

also, i'm seeing this problem in chrome on osx - the memory for the tab started at 36.8Mb and after just a few minutes (maybe 5) it was at 176Mb. even with the patch this continues to happen so we should understand the cause of this problem and fix the real issue.

i would be more inclined to support your patch if it was able to at least fix IE without adding more surface area to the public API - ie if it didn't need to have the flag. that way we could have a fix for IE now but change or back out of it when we properly understand what's causing the leak.

[Patrick Ruzand]

I understand your concern about this flag/patch, and as I said, I would be the first to drop it for a smarter fix. The thing is, I don't have one.

I have attached below a sample that does what gfx does with the registry but in pure javascript except for dojo/dom,dojo/on and dojo/ready requires. This test case reproduces the issue under IE. I think this is the very same issue we have in gfx.

[Patrick Ruzand]

"pure" javascript testcase

[ben hockey]

fwiw, right now i don't have time to look further at this today and i know you're eager to have it in the 1.9 alpha build. ultimately i can't stop you having this patch committed (this is not my component) so you only answer to the owner of the component (whoever that is - i don't even know any more, maybe it's you).

more generally i'm concerned that the registry has introduced more problems than it's ever fixed and that's why i'd like to see it work properly.

[Patrick Ruzand]

Replying to neonstalwart:

fwiw, right now i don't have time to look further at this today and i know you're eager to have it in the 1.9 alpha build. ultimately i can't stop you having this patch committed (this is not my component) so you only answer to the owner of the component (whoever that is - i don't even know any more, maybe it's you).

I don't want to force a patch that may not be satisfying. I'm just trying to fix an IE bug, and I have not found another way of doing this. Secondly, you asked for more informations in this ticket, so yes, I am asnwering to you, and indeed the module owner,who should be still Eugene.

[alexandre]

I tested on FF 19.0.2 and the same problem occurs, FF has 2 GB memory in use and stay extremely slow because the memory leak. It's necessary open another bug for this?

Vitorelli.

[Patrick Ruzand]

Replying to alexandre:

I tested on FF 19.0.2 and the same problem occurs, FF has 2 GB memory in use and stay extremely slow because the memory leak. It's necessary open another bug for this?

Vitorelli.

I have never been able to reproduce with FF (but maybe not with FF 19). Did you use the attach test case or another one ?

(btw, no need to create another ticket).

[ben hockey]

i've seen it leak in chrome and here's a video of FF18 on WinodwsXP - ​https://saucelabs.com/tests/8af5f13c06d14f6585180087ba909a65

you can see the memory climb as the code is running but most of it seems to be reclaimed when i stop the code.

[alexandre]

Replying to pruzand:

Replying to alexandre:

I tested on FF 19.0.2 and the same problem occurs, FF has 2 GB memory in use and stay extremely slow because the memory leak. It's necessary open another bug for this?

Vitorelli.

I have never been able to reproduce with FF (but maybe not with FF 19). Did you use the attach test case or another one ?

(btw, no need to create another ticket).

Yes, I used the attached test case. I see the same problem with dojo charts, because it use gfx. I'm using FF 19.0.2 with Windows 7 (x64), my FF has Firebug 1.11.2 installed with dojo plugin.

[Edited]

I made more tests in the following versions:

• 1.7.2: Problem occurs;
• 1.7.3: Problem occurs;
• 1.7.4: OK;
• 1.8.3: OK;

The problem exists in old versions not in the last versions. The problem that I see was in old versions, sorry.

[Patrick Ruzand]

As discussed during the team meeting, we will make the registry optional (which means it breaks backward compat...) since it's impossible to find a fix that works for all browsers (all version of IE leaks differently..). I attach a new patch, which extracts the registry code from dojox/gfx/shape and adds a new dojox/gfx/registry module that needs to be required to plug the registry API. So, an app/lib that needs the registry API must require() this module.

Note that the Silverlight renderer requires the registry, in order to be able to bind the rawNode with the gfx shape (because of the underlying marshalling, only string can be used for this).

[Patrick Ruzand]

make the registry API optional and move it in a new dojox/gfx/registry module. patch by pruzand (IBM, CCLA)

[Eugene Lazutkin]

The patch looks fine, my only concern is a back reference from a node to its shape --- this stuff can leak, especially on IE. I hope you tested the patch for memory leaks thoroughly. If not, get ready for an onslaught of tickets. :-)

[cjolif]

In [30985]:

refs #16684. Modify charting to take into account removal of gfx registry in 1.9 (is optional now). !strict.

[Patrick Ruzand]

In [30988]:

move registry api to its own module, make it optional. fixes #16684 !strict

[Patrick Ruzand]

In [31289]:

fix IE8 leak, refs #16684, thx elazutkin

(backport to 1.8)

[Patrick Ruzand]

In [31295]:

fix IE8 leak, refs #16684, thx elazutkin (backport to 1.7)