Disabledif JavaScript is very inefficient

I'm going to cowardly back away from this issue for the moment as I don't have time to work on it right now and I'm not all that familiar with this particularly beautiful section of Moodle's heritage.

Reassigning back to moodle.com. If I get a chance, I'll pick it up again but I'm super busy between now and 2.4 freeze.

Just to add a note - the description might give the impression that this problem is only serious in IE, but in fact the page is also horrifically slow in Firefox or Chrome when using a large course. It will typically show the 'this JavaScript is taking ages, do you want to stop it?' box once or twice.

There is a workaround: when users here need to backup sites with standard Moodle backup interface, they typically disable JavaScript so they can make it through this page.

I'm not offering to fix it either, sorry Just thought the workaround might be helpful...

This is a big issue for large courses in the backup UI as well as when using Tim's 'edit dates' contrib report. (The bowser can lockup for literally minutes with the later.)

Results of profiling lib/form/form.js :

1. elementsByName() does a full element traversal for every element, so it's quadratic in the number of elements
2. checkDependencies() updates every dependency on the form for every event. This is really unnecessary as long as we store a bit of state information.
3. _disableElement() makes an expensive ancestor() call which causes significant overhead on initial page load if many items are disabled.

I've rewritten form.js to use a more efficient algorithm and to store more state so that fewer elements need to be updates on events. The improvement is enormous for large forms!

I've tested on recent versions of FF and Chrome, as well as IE 6, 7 and 9

Any idea who should peer review this?

I think Andrew Nicols would be a good person to review this if he has the time.

Andrew, if you are busy, just remove your name from the peer-reviewer field again.

Cheers Tim,

I can look at Peer Reviewing this next week, but I'd also appreciate Sam's thoughts.

I don't know this section of code particularly well or what it is trying to achieve, but I wonder whether we can look at adjusting the HTML to use some more suitable selectors, and to store a mapping of dependent to dependees during instantiation and try to make the JS lazier.

At present, even after the above changes, there are several (one to three) 'on' event handlers created for each dependent element and there's a lot of additional JS in the initializer for each element.

If we moved to this model, we could have a single set of delegated event handlers:

input[type=button|submit|radio|checkbox].hasdependency - click
input.hasdependency:not[type=button|submit|radio|checkbox] - blur
select.hasdependency - change
!(input|select).hasdependency - click, blur, change

This would obviously need some thought, but it should reduce the overhead - particularly for complex forms such as backup.

Andrew

I've put a fix together which:

• adds the hasDependency class to any dependency
• adds a single delegated listener for .hasdependency on click, blur, and change
• checks the type of input together with the type of event and proceeds according to the existing state of affairs

I have however discovered a bug in some part of this approach so I'll keep looking tomorrow.

Tracked the bug down.

Hi Andrew,

Didn't have time to look at this sorry, ended up spending more time on the shifter idea.
Will try again on Monday (I'm away for the rest of the week).

Many thanks
Sam

Hi Andrew,

I like the change, as it cleans up the event handling and cuts down on the javascript on load time. I'd like to point though out that the major inefficiencies I found from profiling were related to the other items that I listed above. Is your plan to merge your changes with the ones that I provided?

Thanks,
-matt

Aha, nevermind. I see that you based the patch off mine. Many thanks!

Hi Andrew,

Both 'dateselector' and 'duration' form element types need the same special processing that your patch does for the datetimeselector. But for all three, shouldn't disabledIf() already handle adding the hasDependency class? It shouldn't be necessary to add that class in the element types themselves.

Thanks Matt,

I suspected that other types may need this too. They do still need it because they're sub-components of the form and don't seem to have the same dependency chain - this is of course, arguably a bug in itself.

I'll get these added as soon as I get a chance.

Andrew

Sam said he'd take a look at this on a peer review front.

In case anyone needs a fix for this now: We're running the original patch I provided

Pull Master Diff URL: https://github.com/mpetrowi/moodle/compare/master...MDL-35672-form-dependencies
Pull Master Branch: https://github.com/mpetrowi/moodle/tree/MDL-35672-form-dependencies

on a large production site with no problems at all. My patch addresses most of the performance problems. The remaining issues in this ticket are related to event delegation, which isn't as critical for performance. The last time I tested there were still issues with the delegation part, see my comments above.

Hi Andrew + Matt,

I've been looking over this now.
Changes look real good thanks. What I noted:

• The duration + dateselector fields using the optional argument do both need updating.
• Undefined variable (simpletypo) form.js line 132, change => changed.

Gets a +1 from me as soon as they are addressed.

Perhaps of interest I created a slightly larger test form for this issue, I'll attach it after this, up to you if you include it in the test instructions etc.
If you use this form you should add a step to change the values and then compare the before after data that is printed when this form is submit.

As for where this should land, there is a chance that this change will cause regressions. Either because people have hacked core, or much more likely due to custom field types.
100% this should land to master, do we consider backporting it or are we all happy with master presently?

Many thanks
Sam

Attached my test form.

I think this is a classic case where this should land in master, then after a month or so, if there are no regressions, it should be back-ported. (The performance in IE is on large forms is really painful. We should back-port once we are sure.)

Thanks Sam, your test form is great! I'll include it in the testing instructions.
I've made the changes you noted and tested on firefox, chrome, and IE 6,9.

I think this is ready for master.

Sorry, this fell off my radar for one reason or another.
I've moved it into integration immediately.

This is currently conflicting on master - showadvanced has been moved to a yui module as part of the shortforms changes.

Could please resolve these conflicts and provide an updated branch Matt ?

Thanks, Damyon

Taking this out of integration for this week. Pleas resubmit when the conflicts on master have been resolved.

Thanks!

Moving this reopened issue out from current integration. Please, re-submit it for integration once ready.

Sure, I'll update the code and re-submit.

Hi Sam,

Yikes, I didn't mean to sit on this for a month. I think it is ready for integration. The changes since the last time you looked at it are only related to to showadvanced moving out of form.js. Thanks!

This will take a while to be backported to stable branches, so in case anyone is impatient, I've attached the form.js file which I've been running on a large production site for the past 4 months. This is a drop-in replacement for lib/form/form.js and includes some of the more critical performance changes. I'm using it with both 2.3.x and 2.4.x sites.

Hi Sam, Have you had a chance to look at this? I'd like to get this integrated as the performance difference is huge for large forms. Thanks.

Any chance of resuscitating this issue? Would be great to get it finished.

Tim, can you suggest someone to peer review this? This issue has been stalled on peer review for months.

Hi Matt,

I'm happy to Peer Review this. However, I think it's probably best if you kill off my commit on there - we can look at integrating those changes separately to this issue.

Cheers,

Andrew

Hi Andrew, I'm not sure I understand as I thought your changes made a lot of sense and relate directly to the Description of the issue. What's the reason for separating them out?

Hi Matt,

You suggested in one of your previous comments that the biggest performance issue was addressed by your changes. I agree that it would be good to get the changes I've proposed in too, but I think that due to a combination of the complexity of both issues taken independently, and the different areas that they address, I feel it would be better to address each individually.

Your change should bring us a quick win that we can commit now and possibly even backport to stable versions; whereas my change is much more complicated as it requires each type of input element to have a different set of event handlers. It may not be possible to backport this change because it may require custom code if there are any custom input types to deal with.

Does that make sense?

Cheers,

Andrew

Hi Andrew, That makes sense. I've killed your commit and put you in as peer reviewer. When you're satisfied, I'd suggest this go into the stable branches as well (I'll do the rebase as there's a trivial change needed.) I've been running the code on 20k+ user 2.3 and 2.4 production sites since December without issues.

[N] Syntax
[N] Whitespace
[N] Output
[-] Language
[-] Databases
[Y] Testing
[Y] Security
[-] Documentation
[Y] Git
[Y] Sanity check

Syntax

checkDependencies - I'd prefer it if you swapped the parameters to the checkDependencies function such that it is more in keeping with other event handlers.
Also, the event passed in is an EventFacade.

Output

group 1: top 'Enabled' checkbox does not disable the Calendar input button
This could be an existing issue, and if so we need to raise a new issue for it.

Whitespace

A few cases of trailing whitespace.

Otherwise, this looks good.

Hi Andrew,

I've addressed the syntax and whitespace issues.

The issue with the calendar popup not getting disabled is an existing problem and the fix is not obvious to me. I think it should be raised as a new issue.

Thanks,
-matt

Thanks Matt,

Sorry for not doing anything with this sooner. I hadn't realised it was up for PR again.

So as not to delay things further, I've rebased against this week's master and pushed (this preserves your authorship of course).

Submitting for integration review now.

Must create an issue to track the calendar not disabled bug discovered here. Battery about to die otherwise I'd do it now.

Hey Andrew - reminder!

Must create an issue to track the calendar not disabled bug discovered here. Battery about to die otherwise I'd do it now.

Integrated to master, thanks Matt!

Clearing the text in the first textfield should disable the Basic and Array groups.
Adding text to the second textfield should disable the Basic and Array groups.

One oddity with this. It appears that we're only enabling/disabling on loss of focus. Makes it a little bit odd. You type and nothing happens and you have to click away to get anything to happen.

Another oddity I noticed (although on further investigation this was NOT introduced by this issue). Using Clean in Chrome checkboxes are grey the same as disabled controls. They're not actually disabled. They just look like they are. https://tracker.moodle.org/secure/attachment/33638/ChromeCheckbox.png

Anyhow, passing.

Oh, tested in Firefox and Chrome.

Thanks again for another week of fixes, improvements and testing. These changes have been released to the world.

Cheers, Damyon

Is there any possibility of backporting this to the 2.4 branch as previously discussed in several comments? I'm working with a 2.4.x site that just encountered this problem. Dropping in the form.js attached here seems to have solved this problem for us. Thanks!

I don't think that will happen as 2.4 is only getting security updates now. Replacing lib/form/form.js with the one attached to this issue should work fine though.