Opened 12 years ago

Closed 12 years ago

Last modified 12 years ago

#5060 closed defect (fixed)

ColorPalette extremely slow with IE6

Reported by: jfcunat Owned by: bill
Priority: high Milestone: 1.0.1
Component: Dijit Version: 1.0
Keywords: Cc: jeanfrancois.cunat@…
Blocked By: Blocking:

Description

I have some remarks about the ColorPalette? component. It is extremely slow on IE 6 (compared to the dojo 0.4.3 version). I know there are some new features concerning accessiblity but the thing is that if you refresh your browser serveral times, it is getting worse and worse.

dijit/tests/test_ColorPalette.html

Attachments (2)

ColorPalette.js (8.8 KB) - added by jfcunat 12 years ago.
ColorPalette? enhancement for performance issue
5060.patch (2.3 KB) - added by bill 12 years ago.
patch version of changes

Download all attachments as: .zip

Change History (11)

comment:1 Changed 12 years ago by jfcunat

Status: newassigned

Changed 12 years ago by jfcunat

Attachment: ColorPalette.js added

ColorPalette? enhancement for performance issue

Changed 12 years ago by bill

Attachment: 5060.patch added

patch version of changes

comment:2 Changed 12 years ago by bill

Owner: changed from jfcunat to bill
Status: assignednew

Thanks for the patch. I'll merge this. I assume the real issue was the typematic connects not getting released? (need to check other widgets to see if they have that same problem).

BTW in the future please submit patches as patch files (use your favorite tool to generate the patch file, like tortoise or eclipse or idea). That lets us see what changed.

Was there any reason you moved the color and colorValue declarations outside the loop? I don't see why that would make it faster (although I do see the reason for only having one dojo.Color object).

comment:3 in reply to:  2 Changed 12 years ago by jfcunat

Replying to bill:

Thanks for the patch. I'll merge this. I assume the real issue was the typematic connects not getting released? (need to check other widgets to see if they have that same problem).

BTW in the future please submit patches as patch files (use your favorite tool to generate the patch file, like tortoise or eclipse or idea). That lets us see what changed.

Was there any reason you moved the color and colorValue declarations outside the loop? I don't see why that would make it faster (although I do see the reason for only having one dojo.Color object).

What was really slow was the multiple call do dojo.moduleUrl for the same image ... For the declarations outside the loops I was wondering if we speed up a little bit because we declare them only once instead of several times inside the double loop. But If you say me that it makes no difference, you can let it as they were...

comment:4 Changed 12 years ago by bill

Hmm, I tried this patch; it didn't seem to make a difference. http://download.dojotoolkit.org/release-1.0.0/dojo-release-1.0.0/dijit/tests/test_ColorPalette.html does take a long time to load (about 5 seconds) but it doesn't seem any faster with this patch, and also it doesn't seem to slow down by multiple loading. Are you seeing something different? I am testing on IE6.

comment:5 in reply to:  4 ; Changed 12 years ago by jfcunat

Replying to bill:

Hmm, I tried this patch; it didn't seem to make a difference. http://download.dojotoolkit.org/release-1.0.0/dojo-release-1.0.0/dijit/tests/test_ColorPalette.html does take a long time to load (about 5 seconds) but it doesn't seem any faster with this patch, and also it doesn't seem to slow down by multiple loading. Are you seeing something different? I am testing on IE6.

In fact, I tested my patch with a 0.9 version and I changed it for 1.0. It was really faster in 0.9 than in 1.0 (especially after few refresh). I think I have found a problem in 1.0. We go twice in dojo.addOnUnLoad of _Widget.js file which causes an error at the second time and the _destroyElement is never called, that is why loading is longer and longer in DOjo 1.0 even with my patch. I think the problem is worse than just a problem of ColorPalette? but it is for all templated widgets in IE6 and dojo 1.0.

comment:6 in reply to:  5 ; Changed 12 years ago by jfcunat

Replying to jfcunat:

Replying to bill:

Hmm, I tried this patch; it didn't seem to make a difference. http://download.dojotoolkit.org/release-1.0.0/dojo-release-1.0.0/dijit/tests/test_ColorPalette.html does take a long time to load (about 5 seconds) but it doesn't seem any faster with this patch, and also it doesn't seem to slow down by multiple loading. Are you seeing something different? I am testing on IE6.

In fact, I tested my patch with a 0.9 version and I changed it for 1.0. It was really faster in 0.9 than in 1.0 (especially after few refresh). I think I have found a problem in 1.0. We go twice in dojo.addOnUnLoad of _Widget.js file which causes an error at the second time and the _destroyElement is never called, that is why loading is longer and longer in DOjo 1.0 even with my patch. I think the problem is worse than just a problem of ColorPalette? but it is for all templated widgets in IE6 and dojo 1.0.

I am afraid it comes from loader.js at line 253 dojo._loadModule = dojo.require = function(/*String*/moduleName, /*Boolean?*/omitModuleCheck) It makes the dojo.require goes twice is the same module and execute code inside it twice

Before dojo.require was defined below like this. dojo.require= dojo._loadModule;

comment:7 in reply to:  6 Changed 12 years ago by jfcunat

Replying to jfcunat:

Replying to jfcunat:

Replying to bill:

Hmm, I tried this patch; it didn't seem to make a difference. http://download.dojotoolkit.org/release-1.0.0/dojo-release-1.0.0/dijit/tests/test_ColorPalette.html does take a long time to load (about 5 seconds) but it doesn't seem any faster with this patch, and also it doesn't seem to slow down by multiple loading. Are you seeing something different? I am testing on IE6.

In fact, I tested my patch with a 0.9 version and I changed it for 1.0. It was really faster in 0.9 than in 1.0 (especially after few refresh). I think I have found a problem in 1.0. We go twice in dojo.addOnUnLoad of _Widget.js file which causes an error at the second time and the _destroyElement is never called, that is why loading is longer and longer in DOjo 1.0 even with my patch. I think the problem is worse than just a problem of ColorPalette? but it is for all templated widgets in IE6 and dojo 1.0.

I am afraid it comes from loader.js at line 253 dojo._loadModule = dojo.require = function(/*String*/moduleName, /*Boolean?*/omitModuleCheck) It makes the dojo.require goes twice is the same module and execute code inside it twice

Before dojo.require was defined below like this. dojo.require= dojo._loadModule;

Forget what I've said above -> I found the problem of double loading : it comes from the debugAtAllCosts: true of the test. Remove it and you will see the difference of the ColorPalette? test without or with my patch. It is faster. Check with many reloads on IE 6

comment:8 Changed 12 years ago by bill

Resolution: fixed
Status: newclosed

(In [11493]) Fixes #5060 on 1.0 branch: ColorPalette? inefficient code and memory leak on IE6. Also makes _Templated.js's dojo.addOnUnload() call itempotent since it executes twice when debugAtAllCosts is true (refs #5091).

Thanks to Jean Cunat (Orange-ftgroup CCLA on file) for patch.

comment:9 Changed 12 years ago by bill

(In [11494]) Fixes #5060 on trunk: ColorPalette? inefficient code and memory leak on IE6. Also makes _Templated.js's dojo.addOnUnload() call itempotent since it executes twice when debugAtAllCosts is true (refs #5091).

Thanks to Jean Cunat (Orange-ftgroup CCLA on file) for patch.

Note: See TracTickets for help on using tickets.