Support from Acquia helps fund testing for Drupal Acquia logo

Comments

nod_’s picture

Status: Needs work » Needs review
FileSize
4.68 KB

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.

Status: Active » Needs work

The last submitted patch, core-js-autocomplete-clean-up-1420798-10.patch, failed testing.

nod_’s picture

Hahahaha, wrong patch sorry about that.

nod_’s picture

Status: Needs review » Needs work
Issue tags: -JavaScript clean-up

The last submitted patch, core-js-replace-each-1428524-4.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +JavaScript clean-up

The last submitted patch, core-js-replace-each-1428524-4.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Why are you doing this to me bot? you changed… :'(

nod_’s picture

reroll

katbailey’s picture

Just making a minor change to the patch in #9 to get rid of the unnecessary $.isFunction() check and providing a D7 version as well.

Status: Needs review » Needs work

The last submitted patch, core-js-replace-each-1428524-d7-10.patch, failed testing.

msonnabaum’s picture

Status: Needs work » Reviewed & tested by the community
FileSize
14.68 KB
16.59 KB

#10 looks good to me. Here's what the before/after looks like for me in chrome canary:

Before:

After:

catch’s picture

Issue tags: +Needs backport to D7
nod_’s picture

The perf improvment comes from removing $.isFunction more than the for loop, still, that's pretty cool.

nod_’s picture

Issue tags: +frontend performance

tag

catch’s picture

Version: 8.x-dev » 7.x-dev
Status: Reviewed & tested by the community » Patch (to be ported)

OK 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.

mojolama’s picture

Status: Patch (to be ported) » Needs review
Issue tags: -frontend performance, -Needs backport to D7, -JavaScript clean-up

Status: Needs review » Needs work
Issue tags: +frontend performance, +Needs backport to D7, +JavaScript clean-up

The last submitted patch, core-js-replace-each-1428524-d7-10.patch, failed testing.

nod_’s picture

ok reroll.

There is one difference with the previous patch, in drupal.js we went from if ($.isFunction(behavior[i].attach)) to if (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.

nod_’s picture

Status: Needs work » Needs review
nod_’s picture

Status: Needs review » Needs work

I messed up dashboard.js

nod_’s picture

Status: Needs work » Needs review
FileSize
1.56 KB
14.13 KB

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

Status: Needs review » Needs work
Issue tags: -frontend performance, -Needs backport to D7, -JavaScript clean-up

The last submitted patch, core-js-replace-each-1428524-22-D8-followup.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

Status: Needs review » Needs work
Issue tags: +frontend performance, +Needs backport to D7, +JavaScript clean-up

The last submitted patch, core-js-replace-each-1428524-22-D8-followup.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
mojolama’s picture

Version: 7.x-dev » 8.x-dev

Changing Version to 8.x and resubmitting test.

mojolama’s picture

nod_’s picture

#22 needs to be commited to D8 and D7 testbot is finally happy.

catch’s picture

Version: 8.x-dev » 7.x-dev
Issue tags: +Needs manual testing

I'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.

danillonunes’s picture

There's a reason why jQuery has isFunction instead of just use typeof. According to this comment on jQuery API:

In certain situations in some browsers, things are incorrectly returned as the "function" type, or things that are in fact functions are returned as another type. There are several test cases you can see here: https://github.com/jquery/jquery/blob/master/test/unit/core.js#L370

One example:

var obj = document.createElement("object");

// Firefox says this is a function
typeof obj; // => "function"

Keep in mind these are mostly edge cases, but the reason $.isFunction was made was simply to be positive about something being a function (which can be quite important for the jQuery library itself, maybe not so much for your code).

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).

jcisio’s picture

FileSize
40.99 KB

#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).

socketwench’s picture

Assigned: Unassigned » socketwench

Assigning to self.

socketwench’s picture

Assigned: socketwench » Unassigned

Wow. I can't even get the patch to apply. Unassigning. (Sorry!!!!)

nod_’s picture

Patch for D7 in #22 works and apply.

It fix this issue too #1518012: Every additional function from Array.prototype called on ajax requests.

pounard’s picture

What 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)

RobLoach’s picture

Somewhat related, yet unrelated, if we had Underscore.js, we could use _each().

nod_’s picture

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.

RobLoach’s picture

Status: Needs review » Needs work
Issue tags: +Needs manual testing, +frontend performance, +Needs backport to D7, +JavaScript clean-up

The last submitted patch, core-js-replace-each-1428524-22-D8-followup.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review

D7 patch is green on #22

nod_’s picture

Issue tags: +js-novice

tag

nod_’s picture

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.

Status: Needs review » Needs work
Issue tags: -Needs manual testing, -frontend performance, -Needs backport to D7, -JavaScript clean-up, -js-novice

The last submitted patch, core-js-replace-each-1428524-43.patch, failed testing.

nod_’s picture

Status: Needs work » Needs review
Issue tags: +Needs manual testing, +frontend performance, +Needs backport to D7, +JavaScript clean-up, +js-novice
droplet’s picture

While 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.

RobLoach’s picture

Status: Needs review » Needs work

I'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.

diff --git a/misc/drupal.js b/misc/drupal.js
index 83b0884..517d3eb 100644
--- a/misc/drupal.js
+++ b/misc/drupal.jsundefined

Broken patch? Missing /core/.

+++ b/modules/dashboard/dashboard.jsundefined
@@ -211,9 +211,9 @@ Drupal.behaviors.dashboard = {
+      for (var i = 0, il = blocks.length; i < il; i += 1) {
+        order.push(region + '[]=' + blocks[i]);

We could probably replace all the instances of jQuery.each() too.

nod_’s picture

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.

RobLoach’s picture

RobLoach’s picture

Status: Needs work » Needs review

I went through and tested:

  • Machine name
  • States
  • Dashboard
  • File upload
  • Fields
  • Overlay

Looks pretty good to me, anything missing?

nod_’s picture

Version: 7.x-dev » 8.x-dev
Status: Needs review » Fixed

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!

tim.plunkett’s picture

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.

Kiphaas7’s picture

Status: Closed (fixed) » Needs work

Sorry 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.

nod_’s picture

Status: Needs work » Closed (fixed)

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.

Kiphaas7’s picture

Right, 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!

jcisio’s picture

Status: Closed (fixed) » Active
FileSize
40.5 KB
45.17 KB

At http://drupalcode.org/project/drupal.git/blob/8.x:/core/misc/drupal.js#l55 we have:

  55   for (i in behaviors) {
  56     if (behaviors.hasOwnProperty(i) && typeof behaviors[i].attach === 'function') {
  57       behaviors[i].attach(context, settings);
  58     }
  59   }

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.

20120802113823.png

20120802114045.png

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.

nod_’s picture

Status: Active » Closed (fixed)

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.

jcisio’s picture

Thanks 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)

nileshlohar’s picture

Issue summary: View changes
FileSize
14.32 KB

I'm aware that we are not committing this to D7.
still rerolling patch in #43 just in case anyone wishes to apply.