Opened 9 years ago

Closed 6 years ago

#10522 closed defect (patchwelcome)

Possible DataGrid Performance Problem..

Reported by: robwaite Owned by: Evan
Priority: high Milestone: future
Component: DojoX Grid Version: 1.4.0
Keywords: Cc:
Blocked By: Blocking:

Description

We have a page with up to 8 datagrids holding large inventories (say 2000 items with 6 columns shown in the grid, backed by a itemfilereadstore). Only one can be seen at at time as we are using a stackpanel.

An auctioneer navigates to various items which causes our grid to scrollToRow(). Normally the 8 grids point to 8 different auctions... but in DEV... we generally open 8 of the same lane. This means that as the clerk navigates... all 8 grids were being told to scrollToRow simultaneously... even though 7 of them were not visible.

What I noticed was when we made the move from 1.2.2 to 1.4.... if the auctioneer navigated very quickly (causing each of the 8 grids to scroll to the new row).. Firefox 3.5 would go to MAX CPU and freeze... and potentially catch up after maybe a minute. Other browsers did not seem to flip out.

We made a quick solution to only scroll when a grid is visible... however.. I was curious what caused this and started looking in the code. If I took 1.4's _Scroller.js and replaced it with 1.2.2's... the problem went away. I then did a line by line change/test to see where the hangup was.

I found changeset 16511. If I simply put "inPos" back in the function prototype... the error goes away. I put a console.log at the top of the function and when I use the 1.4 code... it gets called LOTS of times and the delay is obviously stemming from there. When I put "inPos" back in... it does not get called so often.

I noticed that the "buildPage" function calls "preparePage"... but there were no changes to it... it only took two parameters in 1.2.2 and also in 1.4

Perhaps the purpose of "preparePage" was broken long ago.. and this is the fix... and perhaps our page is just too heavy to "work properly". The only thing is... our system has been using 1.2.2 since it came out and thousands of people hit our system a day. So whatever the purpose of "preparePage" is... it did not affect our functionality.

Anyway... I can't give a test page as our grid is buried deeply in our code.. I spent about 40 minutes trying to write a test case but something big has come up here in the office. As far as we are concerned.. we have it resolved for ourselves by simply scrolling only when a grid is visible (our system has only one visible at a time)

I would imagine however... that if people have large numbers of rows... and they have a group of grids that all scrollToRow at the same time... they will notice a very heavy performance hit with this change.

Change History (7)

comment:1 Changed 9 years ago by robwaite

Looking a little further.. it seems that preparePage() as it was in 1.2.2 always had inReuseNode set as undefined. This would in turn cause createPageNode() to be called always and never call invalidatePageNode(). When "inPos" was removed from the prototype... inReuseNode started having a valid value... and invalidatePageNode() starts being called more regularly.

I noticed that Ticket #9705 mentions cleanNode() which is called by invalidatePageNode(). Ticket #9705 mentions that cleanNode iterates though all widgets and could cause a performance hit.

This might be why our page has such a performance hit since we have LOTS of widgets on the page. Perhaps if I had made a simple test page... I would not have been able to reproduce it unless I added lots of widgets to the page.

I suppose that 1.2.2 could have wasted memory by always creating new page nodes. We however have had no problems with crazy excess memory even though we have our page jam packed full of various widgets and a group of datagrids that have fairly large numbers of rows.

comment:2 Changed 9 years ago by robwaite

Okay.. this will be the last message... sorry for spreading it out. I went into invalidatePageNode() and commented out "cleanNode(p)". This did NOT make the performance issue go away.

Here is an interesting test case... two lines before cleanNode(p), inside of invalidatePageNode()... there is the following line:

delete inNodes[inPageIndex];

I ran two tests... I changed that line to:

console.log("mytest", inNodes[inPageIndex]); delete inNodes[inPageIndex];

When I did this... I would see output like the following:

mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;"> ....

I would see maybe 50 lines of this

When I changed the line to :

console.log("mytest", inNodes[inPageIndex]); delete inNodes[inPageIndex];

I would see "mytest <div role="presentation" style="position: absolute; left: 0pt; top: 0px;">" Maybe once or twice... and then it would stop showing up. It seems like whatever is causing this to get called so much is slowing things down (at least in my environment).

comment:3 Changed 9 years ago by Nathan Toone

Owner: Nathan Toone deleted

Unassigning my tickets.

comment:4 Changed 8 years ago by evan

Milestone: tbdfuture
Owner: set to evan

comment:5 Changed 8 years ago by Evan

Owner: changed from evan to Evan

comment:6 Changed 6 years ago by bill

DojoX Grid and EnhancedGrid are deprecated in favor of dgrid and gridx.

You should upgrade your code to use one of those two grids.

We will consider patches to the old DojoX Grid code though.

comment:7 Changed 6 years ago by bill

Resolution: patchwelcome
Status: newclosed
Note: See TracTickets for help on using tickets.