Opened 11 years ago

Closed 11 years ago

Last modified 11 years ago

#7788 closed defect (fixed)

[patch]Performance-regression in Dojo 1.2

Reported by: SteinM Owned by: Nathan Toone
Priority: high Milestone: 1.2.1
Component: Dijit Version: 1.2beta
Keywords: performance Cc:
Blocked By: Blocking:

Description

There is a performance-regression in Dojo 1.2 and it is manifesting itself in NumberTextBox? and I've attached a simple test to demonstrate the case.

The profiler in Firebug shows that the method buildGroupRE in RegEx?.js gets called over 15 000 times for this simpled test compared to around 1 000 in Dojo 1.1.1.

I've havent done any investigation any further than what Firebug tells me about number of calls and calltime-percentage.

The testcase demonstrates resetting fields and inserting values into som NumberTextFields? and ValidationTextBoxes? and in IE7 the task of inserting values into 20 NumberTextBox?-fields have increased from around 100 milliseconds in 1.1.1 to around 900 milliseconds in 1.2.

Attachments (2)

test_numbertextbox_performance.html (6.9 KB) - added by SteinM 11 years ago.
7788_formValidStatePerformance-1.2.patch (10.0 KB) - added by Nathan Toone 11 years ago.
Single patch file (against the 1.2 branch) to potentially merge in.

Download all attachments as: .zip

Change History (15)

Changed 11 years ago by SteinM

comment:1 Changed 11 years ago by Adam Peller

Component: GeneralDijit
Owner: changed from anonymous to Douglas Hays

Doug - any idea if something changed in validation? I don't think the underlying number code changed much.

comment:2 Changed 11 years ago by Douglas Hays

Owner: changed from Douglas Hays to nathan

Performance problem caused by the #4692 Form widget change. On every value change to a Form's children-widgets, every child's isValid() is being called.

comment:3 Changed 11 years ago by Nathan Toone

Owner: changed from nathan to Nathan Toone

comment:4 Changed 11 years ago by Nathan Toone

Milestone: tbd1.2.1
Priority: normalhigh
Status: newassigned

comment:5 Changed 11 years ago by Nathan Toone

(In [15416]) Refs #7788 addresses this performance issue in trunk - would like to have someone look over it before committing it to 1.2 branch.

comment:6 Changed 11 years ago by Douglas Hays

Moving the call to this.isValid() to before this.connectChildren() needs to be looked at very closely. I would think that isValid() wouldn't have any children to look at in this case and we need to initialize the _invalidWidgets array in case 1 of the children are invalid on init.

comment:7 Changed 11 years ago by Nathan Toone

The reasoning for placing this.isValid call before connectChildren is to ensure that the _invalidWidgets array is populated before any of the connected children's onChange events fire.

Perhaps, however, it would make more sense to move the isValid call to *inside* connectChildren() (since if we re-call connectChildren, we probably want to make sure that the array is reset as well there).

Thoughts?

comment:8 Changed 11 years ago by Nathan Toone

You are correct - adding children after the fact (and then calling connectChildren) doesn't work with this current change. I have made modifications to the logic, and am checking in another fix.

comment:9 Changed 11 years ago by Nathan Toone

(In [15424]) Refs #7788 - change logic used for tracking valid state so that when connecting new children, our list of valid children is updated.

Changed 11 years ago by Nathan Toone

Single patch file (against the 1.2 branch) to potentially merge in.

comment:10 Changed 11 years ago by Nathan Toone

Summary: Performance-regression in Dojo 1.2[patch]Performance-regression in Dojo 1.2

comment:11 Changed 11 years ago by Nathan Toone

Resolution: fixed
Status: assignedclosed

Fixed in branch in revision [15469]

comment:12 Changed 11 years ago by bill

This actually caused a regression on a form where multiple fields are initially invalid... fixed in [17106] for 1.3 release.

comment:13 Changed 11 years ago by bill

Plus typo fix in [17120].

Note: See TracTickets for help on using tickets.