Opened 9 years ago

Closed 9 years ago

#11420 closed defect (fixed)

Reduce unnecessary changes to node.className

Reported by: Shane O'Sullivan Owned by: Shane O'Sullivan
Priority: high Milestone: 1.6
Component: Dijit Version: 1.5.0b2
Keywords: Cc:
Blocked By: Blocking:

Description

A number of places in dijit change the className attribute on a node more often than necessary. This causes large performance issues in IE6 and IE7, where a redraw is triggered every time the CSS class is changed.

Some functions that can be optimized include:

dijit.layout._LayoutWidget.removeChild

  • changes the className even when being destroyed, slowing down the unloading of a page.

dijit.layout._LayoutWidget._setupChild

  • calls dojo.addClass twice in many situations, when just once will do

dijit._Widget._attrToDom

  • calls dojo.removeClass and dojo.addClass on the same node. This should be factored out to a new method, dojo.replaceClass, which only changes the className once, not twice.

dijit._Widget._setClassAttr

  • same as dijit._Widget._attrToDom

Attachments (6)

className.patch (17.8 KB) - added by Shane O'Sullivan 9 years ago.
Implementation of dojo.replaceClass and usage in Dijit
className.2.patch (18.1 KB) - added by Shane O'Sullivan 9 years ago.
Updated Patch with less object creation and change to string equality
dijitClassNames.patch (10.3 KB) - added by Shane O'Sullivan 9 years ago.
Latest patch, with BorderContainer? check for being destroyed removed
dijitClassNames.2.patch (10.2 KB) - added by Shane O'Sullivan 9 years ago.
Updated patch with changes to dijit._Widget added
dijitClassNames.3.patch (9.9 KB) - added by Shane O'Sullivan 9 years ago.
Reduced complexity of code, removed beingDestroyed check in _LayoutWidget
dijitClassNames.4.patch (10.8 KB) - added by Shane O'Sullivan 9 years ago.
Added tests for usage of an empty string with dojo.replaceClass

Download all attachments as: .zip

Change History (13)

Changed 9 years ago by Shane O'Sullivan

Attachment: className.patch added

Implementation of dojo.replaceClass and usage in Dijit

comment:1 Changed 9 years ago by Sam Foster

I have also seen dojo's className changes implicated as a performance problem - in Dynatrace Ajax Edn - but can you observe or measure any difference with these changes?

+1 to idea of dojo.replaceClass. Cheeky implementation with the fake node :) Teensy nit - in dojo/_base/html.js when you compare old and new classNames, you are comparing string equality, so why not !== (instead of !=).

Changed 9 years ago by Shane O'Sullivan

Attachment: className.2.patch added

Updated Patch with less object creation and change to string equality

comment:2 Changed 9 years ago by bill

Milestone: 1.5.11.6
Owner: set to Shane O'Sullivan

Good work on trying to improve performance. This change is much too big for 1.5.1 but I'll mark it for 1.6.


As for code correctness:

  • converting multiple removeClass() calls into a single call like
dojo.removeClass(node, "dijitCalendarHoveredDate dijitCalendarActiveDate");

isn't safe, because it won't do the right thing when the node only has one of those classes, or even if it has both classes but not contiguously, ex: node.className == "dijitCalendarHoveredDate foo dijitCalendarActiveDate".

dojo.removeClass(node, ["dijitCalendarHoveredDate", "dijitCalendarActiveDate"]);

is correct

  • similarly, converting two addClass() calls into one
dojo.addClass(node, "foo bar")

call can result in the same class name being added to a node multiple times, since the "does class already exist on node?" regex check won't work correctly.

dojo.addClass(node, ["foo", "bar"]);

is correct.

  • like Sam said there's no need for a fake node in dojo.removeClass(). Probably what you want is a function
dojo.addRemoveClasses(node, oldClasses, newClasses)

method that combines the code from dojo.removeClass() and dojo.addClass(). Then both dojo.addClass() and dojo.removeClass() could call that new function.

  • in BorderContainer, you are trying to optimize what happens on page unload, but branching on if(this._beingDestroyed) isn't technically valid because the BorderContainer may be destroyed programatically long before page unload, and it's children not destroyed. When BorderContainer is destroyed in this case, it's supposed to remove the class name changes it made on the children

As you can see a change like this needs a lot of testing too, as it can have many subtle bugs.


As for performance, I'm not actually seeing a significant difference on my IE6 though. I tested themeTester.html before and after the change, with these results:

before: 7203, 6844, 6843, 7172, 7094
after: 5641, 6593, 7328, 7375, 7500

(Methodology: started new IE6 browser, cleared cache, then reloaded themeTester.html 5 times. Shut down IE6, reopened it, cleared the cached, then reloaded themeTester.html w/the code changes 5 times)

comment:3 Changed 9 years ago by bill

PS: oh, looks like addClass() and removeClass() call str2array, which automatically calls split() on strings with spaces, so that dojo.removeClass(node, "dijitCalendarHoveredDate dijitCalendarActiveDate") will work. Still probably better to pass in an array though?

comment:4 Changed 9 years ago by Shane O'Sullivan

Status: newassigned

Changed 9 years ago by Shane O'Sullivan

Attachment: dijitClassNames.patch added

Latest patch, with BorderContainer? check for being destroyed removed

Changed 9 years ago by Shane O'Sullivan

Attachment: dijitClassNames.2.patch added

Updated patch with changes to dijit._Widget added

comment:5 Changed 9 years ago by Shane O'Sullivan

Added a patch, adding more usage of dojo.replaceClass where both addClass and removeClass were used.

Changed 9 years ago by Shane O'Sullivan

Attachment: dijitClassNames.3.patch added

Reduced complexity of code, removed beingDestroyed check in _LayoutWidget

Changed 9 years ago by Shane O'Sullivan

Attachment: dijitClassNames.4.patch added

Added tests for usage of an empty string with dojo.replaceClass

comment:6 Changed 9 years ago by Shane O'Sullivan

(In [22758]) Refs #11420 Replaces instances in Dijit where both dojo.addClass and dojo.removeClass were used with the new function dojo.replaceClass. This performs more quickly on IE6 and IE7, as it modifies className just once rather than multiple times, thereby causing fewer page reflows !strict

comment:7 Changed 9 years ago by Shane O'Sullivan

Resolution: fixed
Status: assignedclosed

All instances of multiple unnecessary changes to className in Dijit have been optimized to use dojo.replaceClass, closing the ticket as Fixed.

Note: See TracTickets for help on using tickets.