Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
#1415788: Javascript winter clean-up
Because for
it is much faster and $.each()
changes the loop scope and doesn't filter properties which causes bugs (in autocomplete for example).
Comment | File | Size | Author |
---|---|---|---|
#60 | replace_all_each-1428524-60.patch | 14.32 KB | nileshlohar |
#57 | 20120802113823.png | 45.17 KB | jcisio |
#57 | 20120802114045.png | 40.5 KB | jcisio |
#43 | core-js-replace-each-1428524-43.patch | 14.27 KB | nod_ |
#32 | attachbehaviors-perf.png | 40.99 KB | jcisio |
Comments
Comment #1
nod_This benchmark for vs $.each tells the whole story. Since that touches
Drupal.attachBehaviors
that's a pretty big deal for performance. I'd also like to replace a couple of$(selector).each()
but that's out of scope here.Some might say it's ugly, I'd say its fast and safe. Also some code should be refactored to be less ugly, it was before, don't blame the filtered for.
For review purpose, a big chunk got reindented, might want to ignore whitespace at first.
Comment #3
nod_Hahahaha, wrong patch sorry about that.
Comment #4
nod_reroll because of #1065626: Machine name not editable if every character is replaced.
Comment #6
nod_#4: core-js-replace-each-1428524-4.patch queued for re-testing.
Comment #8
nod_Why are you doing this to me bot? you changed… :'(
Comment #9
nod_reroll
Comment #10
katbailey CreditAttribution: katbailey commentedJust making a minor change to the patch in #9 to get rid of the unnecessary $.isFunction() check and providing a D7 version as well.
Comment #12
msonnabaum CreditAttribution: msonnabaum commented#10 looks good to me. Here's what the before/after looks like for me in chrome canary:
Before:
After:
Comment #13
catchComment #14
nod_The perf improvment comes from removing $.isFunction more than the for loop, still, that's pretty cool.
Comment #15
nod_tag
Comment #16
catchOK this looks great and I've committed/pushed it to 8.x.
It'd be good to document $.each() vs. the for loop at http://drupal.org/node/172169 or somewhere similar if coding standards isn't the right place for it.
Moving to 7.x for backport.
Comment #17
mojolama CreditAttribution: mojolama commented#10: core-js-replace-each-1428524-d7-10.patch queued for re-testing.
Comment #19
nod_ok reroll.
There is one difference with the previous patch, in
drupal.js
we went fromif ($.isFunction(behavior[i].attach))
toif (behaviors[i].attach)
that's what is in Drupal 8.Since i don't want to deal with change notice and breaking contrib I put the function check back but only with
typeof
, which is much faster than jQuery's$.isFunction()
.So basically, this new patch: all the goodness, no api change.
Comment #20
nod_Comment #21
nod_I messed up dashboard.js
Comment #22
nod_New patch for D7.
The bug is in D8 as well, I attached a followup patch. I put back the function check in drupal.js as well, don't want to deal with api change at that level here is a bench showing that $.isFunction slooow: http://jsperf.com/isfunction-vs-typeof-function
Comment #24
nod_#22: core-js-replace-each-1428524-22-D8-followup.patch queued for re-testing.
Comment #26
nod_Comment #27
mojolama CreditAttribution: mojolama commentedChanging Version to 8.x and resubmitting test.
Comment #28
mojolama CreditAttribution: mojolama commented#22: core-js-replace-each-1428524-22-D8-followup.patch queued for re-testing.
Comment #29
nod_#22 needs to be commited to D8 and D7 testbot is finally happy.
Comment #30
catchI've committed the D8 follow-up, moving back to CNR against 7.x for that patch. Also tagging 'needs manual testing' so we can run dashboard and others through their paces with the 7.x patch.
Comment #31
danillonunes CreditAttribution: danillonunes commentedThere's a reason why jQuery has isFunction instead of just use typeof. According to this comment on jQuery API:
Actually, I don't think we need to check if it's a function at all, as was done in D8, but is important to keep in mind that change from $.isFunction to typeof is not a faster way to have the exact same behavior (if that is what is important in D7).
Comment #32
jcisio CreditAttribution: jcisio commented#12 I just want to add that Drupal.attachBehaviors takes itself 0 ms, so performance gain is not measurable. Tested in admin/structure/block page (Drupal.behaviors.length = 6).
Comment #33
socketwench CreditAttribution: socketwench commentedAssigning to self.
Comment #34
socketwench CreditAttribution: socketwench commentedWow. I can't even get the patch to apply. Unassigning. (Sorry!!!!)
Comment #35
nod_Patch for D7 in #22 works and apply.
It fix this issue too #1518012: Every additional function from Array.prototype called on ajax requests.
Comment #36
pounardWhat about JSLint with the big one (for D7)? If I remember well, it doesn't like one or two things we can see a lot in this patch:
- Multiple var statements per function
- for (var something;;)
Aside of that, I guess it's a matter of manual testing more than in review, I like the idea and read the patch, and didn't see anything really awful (I'm no expert in JS thought)
Comment #37
RobLoachSomewhat related, yet unrelated, if we had Underscore.js, we could use _each().
Comment #38
nod_Don't repeat it but the evil plan is to use underscore to help get rid of jQuery in core JS files.
For D8, as a library, underscore is much more useful than jQuery for core use. That's in the future though. We're still loading jQuery and loading underscore on top of that, well, that's just mean :)
Anyway, happy to take the conversation elsewhere, let's not side-track this one too much.
Comment #39
RobLoach#22: core-js-replace-each-1428524-22-D8-followup.patch queued for re-testing.
Comment #41
nod_D7 patch is green on #22
Comment #42
nod_tag
Comment #43
nod_was checking if a reroll was needed, doesn't looks like it. Uploading again to avoid issues when running the testbot. Filesize different because of a comment added in the meantime.
Comment #45
nod_#43: core-js-replace-each-1428524-43.patch queued for re-testing.
Comment #46
droplet CreditAttribution: droplet commentedWhile adding any JS file through drupal_add_js, drupal.js is also loaded?
for..in used in drupal.js too. If forEach is better for performance (&readability), can we write a custom function there?
It is about 20 lines in __underscore.
Comment #47
RobLoachI'd rather not fork individual functions from Underscore. If we bring Underscore in, then let's bring Underscore in. The first step though, is reducing our load on jQuery.
Broken patch? Missing /core/.
We could probably replace all the instances of jQuery.each() too.
Comment #48
nod_That's the D7 backport, it's already in D8.
And yes, I agree but that's out of scope for this issue. Open another issue if you want, i'll make later on otherwise.
Comment #49
RobLoachOh! Haha, sorry about that. #1660952: Replace all jQuery.each() with filtered for loop is it!
Comment #50
RobLoachI went through and tested:
Looks pretty good to me, anything missing?
Comment #51
nod_All right, like a few other recent issues, we're not backporting that.
Sorry to pull the carpet under your feet Rob, I'm happy you review it and found out some more replacement we could do. I'd rather have you on D8 patches instead of those time-consuming-maybe-nothing-will-break patches.
Thanks and see you in D8 queue!
Comment #52
tim.plunketttags
Comment #54
Kiphaas7 CreditAttribution: Kiphaas7 commentedSorry about re-opening, but thought this was the best action here:
So why did $.each get replaced in certain parts by a for..in loop?
http://jsperf.com/jquery-each-vs-for-loop/5
For..in is even slower than $.each(), unless those tests aren't comparable to what is done ehre. At any rate, I'm kinda suspicious of for..in.
Comment #55
nod_because that's what should be there. check jquery source, this is a good change and no variation of jquery each is going to be faster.
Also the jsperf page, that don't test the right things. you should be comparing the $.each with the for loop, not the for in loop. bad benchmark.
Comment #56
Kiphaas7 CreditAttribution: Kiphaas7 commentedRight, I was thinking they were doing some fancy conversion from object to array, then looping the array with a regular for loop. Guess I was wrong!
Comment #57
jcisio CreditAttribution: jcisio commentedAt http://drupalcode.org/project/drupal.git/blob/8.x:/core/misc/drupal.js#l55 we have:
This is a for ... in loop. According to http://jsperf.com/jquery-each-vs-for-loop/5 this (4th line) is the slower the $.each (1st line). Benchmarks seem consistent, too.
The Drupal.behaviors array is small, improvement is not measurable (my comment #32) and comes from elsewhere (nod_'s comment #14). I think we should switch to a traditional for (i < length) loop.
Comment #58
nod_Guys, I appreciate the attention but this is done and burried. The thing that's looped over in the jsperf page is wrong. behavior is an object, we just can't do a for loop, it has to be a for in loop.
Seriously, just put your attention to open issues. Thanks.
Comment #59
jcisio CreditAttribution: jcisio commentedThanks nod_. #58 is correct. Benchmark approves that ($.each is 73% slower):
http://jsperf.com/jquery-each-vs-for-loop/120
(result http://dev.jcisio.com/snap/20120802123750.png)
Comment #60
nileshlohar CreditAttribution: nileshlohar commentedI'm aware that we are not committing this to D7.
still rerolling patch in #43 just in case anyone wishes to apply.