Memory leak using setStore() on dojox.grid.DataGrid

classic Classic list List threaded Threaded
17 messages Options
Reply | Threaded
Open this post in threaded view
|

Memory leak using setStore() on dojox.grid.DataGrid

irialcon
Hi,

We use dojox.grid.DataGrid with dojo.data.ItemFileWriteStore.
We are seeing a large memory leak when we set the store repeatedly
[using setStore()] .

Is this a known issue ?
I have seen some similar posts, but none that fully match:
http://trac.dojotoolkit.org/ticket/11310 (ours is different as we also see the issue on FF)
http://trac.dojotoolkit.org/ticket/11370 (we have a different store implementation..)
http://trac.dojotoolkit.org/ticket/9868

The attached file memoryLeakTest.html contains a stripped down version of our actual code.

Other points to note
- In our 'real code' we update a table at 20 second intervals but we see the same issue , albeit over a longer period
- If we comment out the setStore(..) line in the attached example, no memory leak is seen
- We see the same issue if we use ItemFileReadStore
- Same behaviour seen on FF 3.5.6 and IE 8

Relevant Versions:
DOJO release 1.4.3

Any help appreciated as this is a blocking issue for us.

Regards,
Irial

memoryLeakTest.html
Ted
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

Ted
Not that I'm an expert or anything, but I suspect your stores are not
being destroyed. If your goal is to refresh the store from the server,
maybe creating a new one every time isn't necessary?
Given:
var jsonPhaseStore = new dojo.data.ItemFileWriteStore({
data:populatedData });

function updateTable()
{
        console.log("updateTable");
        var store = new dojo.data.ItemFileWriteStore({ data: populatedData});
        dijit.byId('dojoGrid').setStore( store);
}

I would try:
function updateTable()
{
        console.log("updateTable");
        jsonPhaseStore.close();
}

Which should cause a refresh of the store that's already connected to your grid.
Alternatively you could try a destroyRecursive() on the old store before you create the new one.

There might be something in the FAQ link on this page http://www.dojotoolkit.org/reference-guide/quickstart/data/usingdatastores.html

-Ted

On 12/7/2010 1:58 PM, irialcon wrote:

> Hi,
>
> We use dojox.grid.DataGrid with dojo.data.ItemFileWriteStore.
> We are seeing a large memory leak when we set the store repeatedly
> [using setStore()] .
>
> Is this a known issue ?
> I have seen some similar posts, but none that fully match:
> http://trac.dojotoolkit.org/ticket/11310 (ours is different as we also see
> the issue on FF)
> http://trac.dojotoolkit.org/ticket/11370 (we have a different store
> implementation..)
> http://trac.dojotoolkit.org/ticket/9868
>
> The attached file memoryLeakTest.html contains a stripped down version of
> our actual code.
>
> Other points to note
> - In our 'real code' we update a table at 20 second intervals but we see the
> same issue , albeit over a longer period
> - If we comment out the setStore(..) line in the attached example, no memory
> leak is seen
> - We see the same issue if we use ItemFileReadStore
> - Same behaviour seen on FF 3.5.6 and IE 8
>
> Relevant Versions:
> DOJO release 1.4.3
>
> Any help appreciated as this is a blocking issue for us.
>
> Regards,
> Irial
>
> http://dojo-toolkit.33424.n3.nabble.com/file/n2035356/memoryLeakTest.html
> memoryLeakTest.html

_______________________________________________
FAQ: http://dojotoolkit.org/support/faq
Book: http://docs.dojocampus.org
[hidden email]
http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
Hi Ted,

Thanks very much for swift response.
In answer to the points you made/questions you raised.

> but I suspect your stores are not being destroyed.
Agreed, we're just not sure why they aren't !

>If your goal is to refresh the store from the server,maybe creating a new one every time isn't necessary
Actually, we receive different JSON data every refresh period. We had originally looked at looping through the data items and setting one-by-one, but we as we may have 100's/1000's of rows we felt it would be more performant to instantiate a new store.
We currently don't see a method to modify the full dataset in a store in one go (apart from the constructor) - are you aware of one?

> jsonPhaseStore.close();
We have tried this (with/without clearOnClose set to 'true') and neither seems to prevent the leak

>Alternatively you could try a destroyRecursive() on the old store before you create the new one.
Do you mean for the DataGrid widget ?
If at all possible we don't want to tear that down at each update
(and I don't see such a method on the ItemFileWriteStore)

Any other pointers appreciated !
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

Jared Jurkiewicz
Just update the whole dataset in IF/WS.  The docs have an example of how:

http://docs.dojocampus.org/dojo/data/ItemFileReadStore#reloading-refreshing-itemfilereadstore-from-a-data-object-dojo-toolkit-1-4

-- Jared

On Tue, Dec 7, 2010 at 6:29 PM, irialcon <[hidden email]> wrote:

>
> Hi Ted,
>
> Thanks very much for swift response.
> In answer to the points you made/questions you raised.
>
>> but I suspect your stores are not being destroyed.
> Agreed, we're just not sure why they aren't !
>
>>If your goal is to refresh the store from the server,maybe creating a new
> one every time isn't necessary
> Actually, we receive different JSON data every refresh period. We had
> originally looked at looping through the data items and setting one-by-one,
> but we as we may have 100's/1000's of rows we felt it would be more
> performant to instantiate a new store.
> We currently don't see a method to modify the full dataset in a store in one
> go (apart from the constructor) - are you aware of one?
>
>> jsonPhaseStore.close();
> We have tried this (with/without clearOnClose set to 'true') and neither
> seems to prevent the leak
>
>>Alternatively you could try a destroyRecursive() on the old store before
> you create the new one.
> Do you mean for the DataGrid widget ?
> If at all possible we don't want to tear that down at each update
> (and I don't see such a method on the ItemFileWriteStore)
>
> Any other pointers appreciated !
>
> --
> View this message in context: http://dojo-toolkit.33424.n3.nabble.com/Memory-leak-using-setStore-on-dojox-grid-DataGrid-tp2035356p2036978.html
> Sent from the Dojo Toolkit mailing list archive at Nabble.com.
> _______________________________________________
> FAQ: http://dojotoolkit.org/support/faq
> Book: http://docs.dojocampus.org
> [hidden email]
> http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest
>
_______________________________________________
FAQ: http://dojotoolkit.org/support/faq
Book: http://docs.dojocampus.org
[hidden email]
http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
Hi Jared,

Thanks for the info - much appreciated.

From initial tests that appears to alleviate/avoid the memory leak.
However, we have a related question/issue:

Using code based on the example at (http://docs.dojocampus.org/dojo/data/ItemFileReadStore#reloading-refreshing-itemfilereadstore-from-a-data-object-dojo-toolkit-1-4) the table as not being updated

i.e.
function updateTable()
{
..
reloadableStore1.data = populatedData1;
reloadableStore1.close();
..
}

I order to get the table to refresh we needed to do
function updateTable()
{
..
reloadableStore1.data = populatedData1;
reloadableStore1.close();
dataGrid._refresh()
..
}
-> this works (table updated AND memory leak not seen) BUT we're conscious that _refresh is not part of the public API

So we then tried

function updateTable()
{
..
reloadableStore1.data = populatedData1;
reloadableStore1.close();
dataGrid.setQuery({index: '*'});
..
}
-> this updated table BUT a memory leak is seen

Any further ideas/suggestions ?
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

Jared Jurkiewicz
I think setQuery must be keeping caches of stuff previously
seen/loaded.   That's the only guess I can offer as to why it would
leak.  All setQuery does is:

        setQuery: function(query, queryOptions){
                this._setQuery(query, queryOptions);
                this._refresh(true);
        },

So it calls refresh with a boolean true, which has something to do
with how it renders.

Only recommendation I can offer for now is to continue calling
grid._refresh();.   Maybe a grid dev will speak up and explain that
param.

-- Jared

On Wed, Dec 8, 2010 at 12:38 PM, irialcon <[hidden email]> wrote:

>
> Hi Jared,
>
> Thanks for the info - much appreciated.
>
> From initial tests that appears to alleviate/avoid the memory leak.
> However, we have a related question/issue:
>
> Using code based on the example at
> (http://docs.dojocampus.org/dojo/data/ItemFileReadStore#reloading-refreshing-itemfilereadstore-from-a-data-object-dojo-toolkit-1-4)
> the table as not being updated
>
> i.e.
> function updateTable()
> {
> ..
> reloadableStore1.data = populatedData1;
> reloadableStore1.close();
> ..
> }
>
> I order to get the table to refresh we needed to do
> function updateTable()
> {
> ..
> reloadableStore1.data = populatedData1;
> reloadableStore1.close();
> dataGrid._refresh()
> ..
> }
> -> this works (table updated AND memory leak not seen) BUT we're conscious
> that _refresh is not part of the public API
>
> So we then tried
>
> function updateTable()
> {
> ..
> reloadableStore1.data = populatedData1;
> reloadableStore1.close();
> dataGrid.setQuery({index: '*'});
> ..
> }
> -> this updated table BUT a memory leak is seen
>
> Any further ideas/suggestions ?
> --
> View this message in context: http://dojo-toolkit.33424.n3.nabble.com/Memory-leak-using-setStore-on-dojox-grid-DataGrid-tp2035356p2052514.html
> Sent from the Dojo Toolkit mailing list archive at Nabble.com.
> _______________________________________________
> FAQ: http://dojotoolkit.org/support/faq
> Book: http://docs.dojocampus.org
> [hidden email]
> http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest
>
_______________________________________________
FAQ: http://dojotoolkit.org/support/faq
Book: http://docs.dojocampus.org
[hidden email]
http://mail.dojotoolkit.org/mailman/listinfo/dojo-interest
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
Jared,

Thanks for following up..

> Only recommendation I can offer for now is to continue calling grid._refresh();.  
Yeah, that may suffice as a workaround

> Maybe a grid dev will speak up and explain that param.
Hopefully : we'd like to understand this farther and don't feel comfortable using methods not in the public API..
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

zhuxw
Hi, Irialcon, as you expected, I'm a grid dev :-)

This is definitely a defect of DataGrid. The source of the problem is in _FocusManager.js line 120-126:
        _initColumnHeaders: function(){
                var headers = this._findHeaderCells();
                for(var i = 0; i < headers.length; i++){
                        this._connects.push(dojo.connect(headers[i], "onfocus", this, "doColHeaderFocus"));
                        this._connects.push(dojo.connect(headers[i], "onblur", this, "doColHeaderBlur"));
                }
        },
These connects here are not disconnected when the grid is re-rendered, so the leak comes.
Here's the full call stack:
DataGrid.setStore
  DataGrid._refresh(true)//The arg isRender means whether the whole grid is to re-render
    DataGrid._fetch(0, true)//This "true" is also isRender
------>//Waiting for callback...
DataGrid._onFetchComplete
  _Grid.postrender//The grid is re-rendered, so all the header nodes are different now
    _FocusManager.initFocusView
      _FocusManager._initColumnHeaders//But the old connects are not released.

I've fixed this bug by adding a _headerConnects array for these connections, and destroy them before creating new ones:
        _initColumnHeaders: function(){
                dojo.forEach(this._headerConnects, dojo.disconnect);
                var headers = this._findHeaderCells();
                for(var i = 0; i < headers.length; i++){
                        this._headerConnects.push(dojo.connect(headers[i], "onfocus", this, "doColHeaderFocus"));
                        this._headerConnects.push(dojo.connect(headers[i], "onblur", this, "doColHeaderBlur"));
                }
        },

I've tested it on IE7/6, no leak anymore.

Will create a patch for this and find some committer to check in for me :)

Best Regards.
Oliver @ Shanghai
Best Regards
-------------
Oliver
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
Hi Oliver,

Thanks for investigating this further.

Unfortunately we still see a memory leak when we try your suggestion.
- See the attached  _FocusManaer.js which we patched based on your reply (let us know if we missed
anything)
- We ran our tests with the same memoryLeakTest.html (attached to our original posting)
- We ran tests on IE8 and FF3.5.6 and saw significant  memory leaks on each

Qs:
- can you forward the patched _FocusManger.js that you used for your tests (if it differs from what we have attached)
- did you run your tests with the memoryLeakTest.html (attached to our original posting)
-- If not, what was different in your test ?

Thanks & Regards,
Irial

_FocusManager.js
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
Some additional info:

At this point we are presuming that
- Oliver's suggested fix (_FocusManager.js) may fix one leak but that there are definitely additional memory leaks in other code invoked from setStore(..)

To help isolate those I have attached another test file (memoryLeakTest_setQuery_versus__refresh.html)

We have run the following tests using that test file:

1. In each case we use patched _FocusManager.js
(based on Oliver's suggestions and attached to previous post)

2. We run 2 separate tests

   i. dojoGrid._refresh();         uncomment
   Result: No memory leak seen
   Concern: Using non public method/API

   ii. dojoGrid.setQuery({index: '*'});      uncomment
   Result: Memory leak seen
   Advantage: Using public method/API

Let me know if we can provide any useful information for the investigation.

Thanks,
Irial

memoryLeakTest_setQuery_versus__refresh.html
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

geoghegk
This post has NOT been accepted by the mailing list yet.
Hi,
Does anybody have any other ideas on this. Setting the data of the store as described in the previous
post (
http://docs.dojocampus.org/dojo/data/ItemFileReadStore#reloading-refreshing-itemfilereadstore-from-a-data-object-dojo-toolkit-1-4) does certainly improve the memory leak situation but it does not eliminate it. Memory continues to grow when the table is refreshed.
I have also tried deleting and re-adding each item in the store, but this results in an even worse memory leak.
Any ideas welcome.

Thanks
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

zhuxw
In reply to this post by irialcon
Hi, irialcon,
Sorry for these days' delay. :-)

Seems my fix only solved the DOM node leak. The whole memory use is slowly increasing if I keep calling grid._refresh(true);  But no memory leak if calling grid._refresh(false);

Still investigating this, but much harder to find than the DOM node leak. Will let you know if I find anything.


Best Regards.
Oliver @ Shanghai
Best Regards
-------------
Oliver
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

zhuxw
In reply to this post by irialcon
One fix to my previous fix:
_initColumnHeaders: function(){
                dojo.forEach(this._headerConnects, dojo.disconnect);
                this._headerConnects = []; //Should clear this array
                var headers = this._findHeaderCells();
                for(var i = 0; i < headers.length; i++){
                        this._headerConnects.push(dojo.connect(headers[i], "onfocus", this, "doColHeaderFocus"));
                        this._headerConnects.push(dojo.connect(headers[i], "onblur", this, "doColHeaderBlur"));
                }
        },

With this fix, grid._refresh(true) is not leaking, so is grid.setQuery(...);
Still working on grid.setStore(...)

Here's the patched _FocusManager.js: _FocusManager.js

Best Regards
Oliver @ Shanghai
Best Regards
-------------
Oliver
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

zhuxw
In reply to this post by irialcon
Found another leak source:
In DataGrid.js:
        _setStore: function(store){
                if(this.store&&this._store_connects){
// dojo.disconnect will not remove the book-keeping things in _Widget.
// dojo.forEach(this._store_connects,function(arr){
// dojo.forEach(arr, dojo.disconnect);
// });
// Should use _Widget.disconnect to disconnect the handles returned from _Widget.connect
                        dojo.forEach(this._store_connects, this.disconnect, this);
                }
                this.store = store;
                .........
        },
With this fix, grid._setStore is not leaking. So far every function call in grid.setStore is not leaking if called alone:
this._setQuery(...);
this._setStore(...);
this._refresh(true);
But it seems still leaking if they're used together.

Best Regards
Oliver @ Shanghai
Best Regards
-------------
Oliver
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
In reply to this post by zhuxw
Oliver,

We really appreciate you investigating the further.
We will try the changes you suggest and try to verify that we see the same behaviour.

Irial
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

irialcon
In reply to this post by zhuxw
Using the Mozilla Memory Profiler we can see that the the the instances of Arrays grows by the number of items in the table each time you refresh. Unfortunately the Memory profiler doesn't allow you to drill down any further so that is all we got.

The test was run using:
memoryLeakTest2.html
Reply | Threaded
Open this post in threaded view
|

Re: Memory leak using setStore() on dojox.grid.DataGrid

Evan
In reply to this post by irialcon
This issue was fixed by:
1. Oliver's patch
2. Strictly follow Jared's doc for resetting datastore

Please check with #12037