use css id for elements instead of names for fapi [#1577644]

searching for on name attribute is unfortunately /very/ inefficient with jquery. it does a regex search over all elements of the dom. a page with many elements needs an incredibbly long to load. depending on the number of elements and input fields, it can take minutes(with ajax callbacks disabled). a seach for element by id's is fast. now rules are generated after form_build and formbuild adds id's to the elements. so we have them already when rules are generated. the only thing necesarry is to pass them to js.
i tried it an i got a speed increase of faktor 5-10 on firefox. on safary and crome results should be even better(getElementById in particular is faster with jquery).

problem: i found no "simple" and clean way to pass the id. (for testing i just added the id and removed it before adding the rule to the element, so it wouldn't cause any trouble, but thats no good).

every way i can think of to add it needs some rework in many files, so that the id does't get in the way. maybe you have some idea?

Comment	File	Size	Author
#4	clientside_validation.js_.patch	1.58 KB	kaidawai

Comments

Comment #1

attiks commented 14 May 2012 at 14:47

The main reason for using names was (and still is) that jquery.validate doesn't work with id's. So we can speed up the binding of the rules. We'll try to optimize it a bit.

Comment #2

kaidawai commented 14 May 2012 at 18:11

that would be awsome. binding is imo the only place where the difference in performance matters. think it is ~ O( n(rules) * n(dom-elements) * (duplicate calls)).
the impact is that browsers complain about a hanging script if you have something a bigger than a contact form.
any other place imo the difference in performance doesn't matter.
so if you find a way to somehow pass the id to the script: that would be awsome.

Comment #3

attiks commented 14 May 2012 at 19:17

Assigned:

Unassigned

» jelle_s

Most of them are optimised in latest dev version, still using the name, but we optimized the selectors.

Comment #4

kaidawai commented 15 May 2012 at 14:17

Status	File	Size
new	clientside_validation.js_.patch	1.58 KB

hmm i looked in the git but didn't spot the optimizations(both ...31 and ....32. ). i take a closer look.

meanwhile i also thought about optimizing the part and i thought of turning the loop around.
looks better but i am uncertain about attribute lookups in js. new browser got a lot of optimizing compared with old ones so it may not help on some browsers.

it does the dom traversal just once but then does a lookup in rules.

      var rules = self.forms[formid]['rules'];
      $("#" + formid + " :input[name]").each(function(idx,elem){
        var rule = rules[elem.name];
        if(rule){
          elem = $(elem);
          $(elem).rules("add",rule);
          $(elem).change(hideErrordiv);
         }
       });

i attached a patch to clarfy. still...

Comment #5

attiks commented 15 May 2012 at 14:32

That code is already changed, and it looks you removed the part that checks if the element exists. The part that needs extra attention is the 'window.setTimeout' since now it's called for each rule.

    if('rules' in self.forms[formid]){
      var $form_els = $form.find('input, textarea, select');
      jQuery.each (self.forms[formid]['rules'], function(r) {
        // Check if element exist in DOM before adding the rule
        $element = $form_els.filter("[name='" + r + "']");
        if ($element.length) {
          $element.rules("add", self.forms[formid]['rules'][r]);
          $element.change(function(){
            //wait just one millisecond until the error div is updated
            window.setTimeout(function(){
              var visibles = 0;
              $("div.messages.error ul li").each(function(){
                if($(this).is(':visible')){
                  visibles++;
                }
                else {
                  $(this).remove();
                }
              });
              if(visibles < 1){
                $("div.messages.error").hide();
              }
            }, 1);
          });
        }
      });
    }

Comment #6

kaidawai commented 15 May 2012 at 15:11

i have a look at your changes.
but look closer.
instead of

jQuery.each (self.forms[formid]['rules']

i do this:

$("#" + formid + " :input[name]").each(

and that is a loop over an array of elements that all exists.
rule may not exist in this case-
what is the difference:
it changes performance from O(N²) to O(N)
on browsers that do "object.atribute" in O(1)

... tomorrow ...

Comment #7

attiks commented 15 May 2012 at 16:35

I see, but the new code does something similar by storing all elements inside $form_els (var $form_els = $form.find('input, textarea, select');). The ':input' selector is slow because it will loop over all nodes, that's why we changed it.

I'll try to do some performance testing with both your and my code.

Comment #8

attiks commented 15 May 2012 at 19:06

I ran some tests and your code makes a lot of difference for FF, so I used it, thanks.

Test done on a form with 200 elements, FF went from 155ms to 55ms.

      var rules = self.forms[formid]['rules'];
      $form.find('input, textarea, select').each(function(idx, elem) {
        var rule = rules[elem.name];
        if (rule) {
          elem = $(elem);
          $(elem).rules("add",rule);
          $(elem).change(hideErrordiv);
        }
      });

Comment #9

kaidawai commented 16 May 2012 at 08:43

good idea to put both optimizations together!
(new)chrome and safary should show a bigger difference. but old browsers(FF2) are a different story, ... id would still be better .... but well(and your computer seems to be a lot faster than mine).

also the "elem.name" is naked. if there are browser issues elem needs to be expanded first to $(elem) ....

Comment #10

attiks commented 16 May 2012 at 12:17

Test in #8 was done with FF12, for some reason FF12 was really slow (other browsers were done in 60ms). I think 55ms delay for a page with 200 form elements (and all with validation) is acceptable. Switching to Id's will complicate things, so not really sure if it's worth it.