Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Performance in IE #367

Closed
secart opened this issue Apr 28, 2019 · 9 comments
Closed

Performance in IE #367

secart opened this issue Apr 28, 2019 · 9 comments

Comments

@secart
Copy link

secart commented Apr 28, 2019

We're using slick grid in an application with very wide data-sets, typically 150 columns and 100K rows. We are updating from 2.3 to 2.4.7 and noticed a performance issue when using IE11.

The issue stems from the change in the cacheEntry node from a standard DOM object to a JQuery object which appears to have been introduced with the frozen columns updates. On testing the issue is only really prevalent with a large number of columns where not all columns fit within the viewport.

I've isolated two sections of code and applied changes below which has reduced the rendering time from 3 seconds to 400ms in our application. Can you review and apply the same if there are no side effects.

Within the cleanUpAndRenderCells function

         /*
            Replaced below with DOM navigation to improve performance
        */
        var processedRow;
        var node;
        while ((processedRow = processedRows.pop()) != null) {
            cacheEntry = rowsCache[processedRow];
            var columnIdx;
            while ((columnIdx = cacheEntry.cellRenderQueue.pop()) != null) {
                node = x.lastChild;

                if (hasFrozenColumns() && (columnIdx > options.frozenColumn)) {
                    cacheEntry.rowNode[1].appendChild(node);
                } else {
                    cacheEntry.rowNode[0].appendChild(node);
                }

                cacheEntry.cellNodesByColumnIdx[columnIdx] = $(node);
            }
        }

      /*
        var processedRow;
        var $node;
        while ((processedRow = processedRows.pop()) != null) {
        cacheEntry = rowsCache[processedRow];
        var columnIdx;
        while ((columnIdx = cacheEntry.cellRenderQueue.pop()) != null) {
            $node = $(x).children().last();

            if (hasFrozenColumns() && ( columnIdx > options.frozenColumn )) {
            $(cacheEntry.rowNode[1]).append($node);
            } else {
            $(cacheEntry.rowNode[0]).append($node);
            }

            cacheEntry.cellNodesByColumnIdx[columnIdx] = $node;
        }
        }
      */

The try catch within the getCellNode caused delays in IE when not all cells have been cached

    /*
        Replaced below with if condition to reduce error throws and improve performance
    */
    function getCellNode(row, cell) {
        if (rowsCache[row]) {
            ensureCellNodesInRowsCache(row);
            try {
                if (rowsCache[row].cellNodesByColumnIdx.length > cell) {
                    return rowsCache[row].cellNodesByColumnIdx[cell][0];
                }
                else {
                    return null;
                }
            } catch (e) {
                return rowsCache[row].cellNodesByColumnIdx[cell];
            }
        }
        return null;
    }

    /*
    function getCellNode(row, cell) {
        if (rowsCache[row]) {
            ensureCellNodesInRowsCache(row);
            try {
                return rowsCache[row].cellNodesByColumnIdx[cell][0];
            } catch (e) {
                return rowsCache[row].cellNodesByColumnIdx[cell];
            }
        }
        return null;
    }
    */
@6pac
Copy link
Owner

6pac commented Apr 28, 2019

Thanks for this submission, I think avoiding jQuery for the cached rows has always been central to the SlickGrid way. @ghiscoding this is your domain, shall I leave for you or will I take a look?

@ghiscoding
Copy link
Collaborator

ghiscoding commented Apr 29, 2019 •
edited
Loading

Unfortunately not really my expertise, we would need to compare the before and after and see if we have made changes causing this. However I must say that removing something will have most probably have a side effect. I mean, I would argue that before removing anything, we would need to understand why it was implemented in the first place, unless it wasn't there prior to the frozen feature.

@secart
Copy link
Author

secart commented Apr 29, 2019

The suggested code change still results in a JQuery object in the rowsCache, but does not use JQuery to navigate the DOM.

Looking through the code there are multiple changes between 2.3 and 2.4.7 which assume the rowsCache object is a JQuery object which would require review if reverting back to standard DOM object.

@6pac
Copy link
Owner

6pac commented Apr 29, 2019

OK, I'll have to go back and do a 'before' and 'after' review.

@ghiscoding
Copy link
Collaborator

I see what @secart mean now with the jQuery vs native javascript to deal with the DOM. Merging the X-SlickGrid and this lib to get the Frozen feature in, was no small feat I can tell you that and I agree that there was some differences where I was not sure on how to deal with when I worked on that library merging. If we can improve performances without removing any features, I'm all for it.

The Grouping Examples has the button to dynamically add 500k rows, I assume that example would be really bad on IE, is it manageable with the changes shown in top? I rarely deal with that many rows, we use pagination to take only a subset at a time. Anyhow, before we merged and release the Frozen feature, we tested all examples to see if they all worked, before and after, I guess we'll have to redo the same exercise with these potential changes. My time however is a little less than before, I now have extra classes on top of full time working, so I can't bring as much as time.

@secart
Copy link
Author

secart commented Apr 29, 2019

The Grouping example does not have frozen columns or enough columns to exhibit the issue.

I've hacked together an example from the frozen columns sample with 200 columns and 100K rows, see attached. Try this in IE and after loading scroll to the right. You should see a delay before the cells are rendered. Chrome is a little jumpy, but not to the extent of IE.

If you then apply the suggested changes you should see IE is much more responsive and Chrome is very smooth.

example-frozen-columns-large.zip

@6pac
Copy link
Owner

6pac commented Apr 29, 2019

Thanks, that's a great test set. I'm happy to apply your changes pretty much as-is, since there's no change to the data structures. You've just translated from jQuery to native JS.

However I would like to go further and remove jQuery objects from the cache (if that's how it was before the merge). That's another change though, and would have to be carefully tested. I'll make a new issue for that.

@secart
Copy link
Author

secart commented Apr 29, 2019

Thanks,

I think removing JQuery objects from the cache will also address other performance hotspots I'm seeing in the cleanUpCells function, but agree this is a bigger task.

@ghiscoding
Copy link
Collaborator

ghiscoding commented May 30, 2019 •
edited
Loading

@secart
I replicated your code change into PR #385, could you please review the PR and confirm that it is all correct before we merge. I do see the improvement in performance, especially in IE like you said, that is a nice one too.

@secart @6pac
I tested locally with my repos (Angular-Slickgrid, Aurelia-Slickgrid), I have 23 examples there, and I didn't see any issues. I did not test the entire set of SlickGrid examples, but that could be an extra check if needed.

@6pac 6pac closed this as completed in 7408a11 May 30, 2019
6pac added a commit that referenced this issue May 30, 2019
@6pac
Merge pull request #385 from 6pac/bugfix/performance-improvements
d8c360c 
fix(ie): increase performance in IE, closes #367
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
3 participants
@6pac @ghiscoding @secart