Leaky row clean up and non - async post render

[suedama1756]

Hi,

We are currently using a framework similar to knockout that allows us to use declarative data and action bindings in our templates. To bind the data for cells that use complex templates we are using the asyncPostRender callback (with a delay of zero, more about that later...). When rows are cleared from the cache we encounter leaks, the framework we use correctly hooks into the jquery API to receive node clean notifications, however, due to the mechanism used to remove the row the clean-up code is not invoked.

By changing the removeRowFromCache function as follows the leak is fixed:

//$canvas[0].removeChild(node); $(node).remove();

I realize that attaching to the asyncpostrender is not a great way to do what we are doing (I've read your responses to previous posts). We would like a non-async post render for wiring up our bindings. All other aspects of the slick grid work perfectly for us, and the two frameworks are working really nicely together, please can we have a non-async post render? (I realize people could use this to abuse the grid, but it could also be a great extension point).

Thanks

[mleibman]

First of all, let me understand what you are actually asking here.

1) Something is leaking. This is happening because of whatever you're doing in asyncPostRender callback. Either attaching event handlers or using some widgets that require hooking into jQuery.remove(). jQuery.remove() is too slow to be used here, and I am very explicit about doing what I can to prevent people from shooting themselves in the foot. I don't know anything about how you're integrating with your knockout-like framework, so I can't really comment here.

2) You have to hack around and use a 0ms delay with asyncPostRender. Here, as well as above, you know what I'm going to say :)

3) You want an alternative to 2. I'll think about that.

[suedama1756]

In response:

1. We ourselves don't actually need jquery remove as the framework doesn't use jquery.data() as this can be quite slow. The framework does hook into jquery cleandata so that it can respond correctly to the numerous APIs that do use remove. I only suggested remove as the slick code already makes heavy use of jquery dom manipulation and I thought it would be convenient. A destroy row, or similar callback would work fine for our case. That said, I'd be interested in understanding why you think remove is too slow, especially as it would only occur when you actually remove nodes from the cache, and if you haven't added any events, etc. then performance seems fine, and if you have..., well you will want them cleaned.
2. We are in agreement, however, its the only convenient place to get a handle on the cell node. Also, in terms of people shooting themselves in the foot, I think the people you are trying to protect, are also the people that will attach event handlers in the post event anyway and not notice they are leaking :)
3. This would be greatly appreciated, although, it would likely go hand in hand with 1, as the most likely reason for needed the element would be to attach events, or something similar. Of course this behaviour could quite easily be optional (along with the jquery remove).

Failing this being added directly, I'm assuming there is no issue with forking the code for our minor changes?

[mleibman]

1. For performance reasons, I try to keep the core rendering loop as optimized, direct and clean as possible, doing all DOM manipulation directly. Removing rows from the cache, and therefore from the DOM, is actually the slowest part of render loop as you scroll, so that has to be as fast as possible. (It is slower than adding new rows since that is done in bulk, while removal has to be done one-by-one.)
2. I have thought about enforcing a minimum on the asyncPostRender delay :)
3. If a direct DOM decorator were to be added, we'd need a cleanup mechanism as well. Otherwise, the next logical question would be how to get rid of memory leaks. Either way, this can easily kill the performance, so it'd probably be disabled by default and put under an option "yesIKnowWhatImDoingPleaseEnableDecorators" :)

Lastly, yes, that is the nature of FOSS, and is, in fact, greatly encouraged.

[suedama1756]

Thanks for the feedback. I think the option to enable decorators/post renderers is in keeping with your existing API, and probably comes with the same warnings as asyncpostrender. I completely understand your drive to keep performance up, I've been running 100s of different simulations on my own framework to verify best average performance across browsers, but is there a chance that perhaps you don't need quite that much performance, I have to say, we've done some pretty complex stuff in our cell templates and slick grid has never felt tardy (even with my $(node).remove() modification :) ), and we are testing on some pretty out of date kit (and browsers, IE7). That said, I think we can still achieve something that satisfies all through optional callbacks, I'll look at creating a patch :)

I do have a work around that will work without any modification to the slick grid source, but I'm not that happy with it. The option is to background clean the nodes. For example, once I have served up template text for a cell I add it to a list of (row, col) that need post processing, I can then use the getCellNode API to get the cell node and apply my post processing (only one setTimeout call here, rather than potentially one per cell). During post processing I can push the nodes into my own cache, cleaning any in the cache that are no longer available. As I said, yuck, only reason I'm mentioning it is that I think it hopefully strengthens my case for having something built in :)

[suedama1756]

One last thing, can we pick a less wordy option name "yesIKnowWhatImDoingPleaseEnableDecorators", it just adds too many bytes to our download :)

[mleibman]

I've played around with this and actually implemented synchronous "decorators", but decided to scrap that since I realized that it really needs to be async, even if it's 0-delay, in order to not destroy scrolling responsiveness, and the resulting API would be very similar to asyncPostRenderers. So, the plan for now is:

1. Enforce a minimum initial delay on asyncPostRender so that it doesn't interfere with scrolling. Still call it progressively, but allow a 0-delay on progressive calls.
2. Add a mechanism to do clean-ups. This one is more tricky. The easiest way to register the cleanup is probably to return a cleanup function in the asyncPostRender itself since the user has easy access to all relevant variables and can easily wrap them in a closure.

For example:

function testPostRender(cellNode, row, dataContext, colDef) {
  var x = document.createElement('div');
  cellNode.appendChild(x);
  $(x).slider({value: dataContext.percentComplete});
  return function() { $(x).slider("destroy"); }
}

[suedama1756]

Thanks for looking into this, I tried myself to use a 0-delay, however, in out case this caused the screen to flash when re-rendering (inserting items at the top in a twitter type feed, and paging). This is due to the fact that most of the real content in our case is rendered during the "bind" phase which is performed in the post render. Scroll performance will obviously be affected by multiple factors, how expensive is the post render operation, what browser you are running etc.

I can see that trying to get the post render to keep up with scrolling would not be effective, however, I was under the impression that the whole render cycle only occurred once you effectively let go of the thumb so perhaps I'm not fully understanding you on that one.

In terms of clean-up, my previously posted solution actually worked really well for us and required not changes to slick grid. I effectively added any nodes that needed clean-up to a pool, I then verify they are still part of the DOM in the background and clean them up automatically. A bit like a node garbage collector, this in combination with DOM Level 3 events for browsers that support it seems to work really well, and has the added advantage for us in that it'l work with any 3rd party control we integrate with.

,

[evanworley]

Hi Folks,

Bringing back an old topic. I am also encountering this issue, almost exactly. Not so much the memory leak, but the "flashing" ugliness brought about by asyncPostRender bindings. I don't mind the quick flash so much, but the core discomfort comes from the fact that not all cells in a row are bound to a view model. When this happens you have some cells show up "immediately", and then the other cells, which are bound to a view model field, flash in. It's very bad looking.

I've resorted to having formatters on each column which first emit state data, and then when the bindings kick in they take over. However, this doesn't always work. For example with the knockout "css" binding, it's adding css classes based on model field values, but it cannot remove any classes added statically during my bootstrapping. Therefore, if I am adding a class "status-done" where "done" is the dynamic part. If something changes in my view model and the status becomes "released", the item will have classes "status-done" (static) and "status-released" (dyanmically added).

So not only am I writing way too much scaffolding for these simple cells, but it's also not bulletproof.

Any thoughts are extremely appreciated. This is the only blocker to having extremely awesome, fast, tables.