Title says all. Changing this only saves one function call but it saves this call hundreds of times. there is a comment in the jQuery source that assure both are equivalent. It's on the sexy 169 line.
- If a
$(domelement)
is used more than once it goes in a variable with a$domelement
name in the closest scope around it, - removed useless
$()
wrapping that were already jQuery objects, - I fixed two leaking global vars in this patch too (in tabledrag),
- I checked everything but I'm sure sun will end up questioning that ;) be my guest for testing.
If you look closely at the patch you might wonder why context
was passed as an argument to attachBehaviors
instead of $(context)
, saving a few thousand useless calls. I'm wondering too.
And tabledrag needs love badly.
This patch will confilct with the other on about changing .size()
to .length
and most likely the one about eval
in tableHeader
.
Comment | File | Size | Author |
---|---|---|---|
#71 | 1419968-follow-up.patch | 1.08 KB | droplet |
#63 | drupal-1419968-61.patch | 660 bytes | tim.plunkett |
#61 | drupal-1419968-61.patch | 660 bytes | tim.plunkett |
#53 | drupal-1419968-53.patch | 94.07 KB | tim.plunkett |
#49 | tabledrag-49-do-not-test.patch | 25.59 KB | droplet |
Comments
Comment #1
droplet CreditAttribution: droplet commentedFor issue purpose, most of this patch looks good to me. (I've not do real test)
not same ??
should be
why copy again? it returned "return row;" and not use $row anymore.
-28 days to next Drupal core point release.
Comment #2
nod_I just wanted to change the selectors, tabledrag is a huge mess and if I start cleaning it up i'll end up refactoring everything. The change you proposed will not work, try it :þ nextRow it the DOM element $nextRow is the jQuery object of the DOM element.
I'm not changing how and when elements are used even if it's stupid, just selectors. There'll be another issue for that.
Comment #3
sunThe base/reasoning for this issue seems to be frontend performance.
However, looking at the referenced jQuery core lines, that code almost looks like a no-op to me.
As in PHP, the cost of a mere function call is almost unmeasurable in JS either.
So before we're going to do this (which in itself would imply kind of a new JS coding standard), and break many patches in the queue, I'd like to see that there is actually a measurable performance difference.
Comment #4
nod_for perf mesurements, here is a little something : http://jsperf.com/drupal-dom-selector/2 you can play around with it I took the html tree from admin/structure/menu/manage/navigation and the selector that is used for table row styling, I'm much more interested in IE results but don't have any around to test.
I don't care about other patches, I'll reroll this one or others as needed.
Comment #5
nod_With a less obnoxious selector that can be optimized with querySelectorAll, results are better: http://jsperf.com/drupal-dom-selector/4
As you can see by following the link there is a little performance difference, this is not entirely a style issue. Not by a crazy amount but still it adds up.
Comment #6
nod_after a bit more testing http://jsperf.com/drupal-dom-selector/5 there is a 5–10% overhead for
$('selector', domelement);
depending on the browser.Comment #7
droplet CreditAttribution: droplet commentedIt's pattern is widely use in jQuery world:
http://addyosmani.com/jqprovenperformance/
$('selector').find() always better than context
It's the worst case:
http://jsperf.com/drupal-dom-selector/6
Comment #8
nod_reroll
Comment #9
sunIt looks like this can be backported to D7?
This needs manual testing, though not really sure how to approach that.
Comment #10
nod_yeah sure I can help backport if it doesn't apply properly. For testing you'd be testing jQuery so I think the review should be on making sure I haven't done anything else than create vars and replaced selectors.
Comment #11
nod_rereroll, caught a typo and keep up with the latest core changes.
Comment #12
droplet CreditAttribution: droplet commented@nod_,
both you & me are wrong. I made a wrong suggestion on #1
If we simplify the changes, it will looks like this:
http://jsfiddle.net/wk4Qe/
Orig: check on first matched element (FALSE)
Patched: check on all matched element (TRUE)
Comment #13
nod_oh, good catch!
Just needed to add a
.eq(0)
: http://jsfiddle.net/Sj9JW/Comment #14
droplet CreditAttribution: droplet commentedReviewed code, and randomly do tests. It's worked.
Comment #15
catchhttp://jsperf.com/drupal-dom-selector/6 looks good to me there's a regression in opera but a decent improvement in every other browser.
However I agree this needs at least some manual testing, and there's no sign of that here.
Comment #16
nod_rererereroll since lots of little fixes have been committed to JS files lately :D
What really needs to be tested are the changes in tabledrag, the rest of them are really straightforward see the comment about the comment in jquery source file. Seriously if the fixes to tabledrag are removed it's all 1 line changes or maybe two when I create a new var.
Comment #17
catchTagging as novice for the tabledrag testing.
Comment #18
droplet CreditAttribution: droplet commentedLooks find to me.
Following can be a separated issue (remark)
Few places have above pattern.
equivalent to
http://jsfiddle.net/kyJzh/
http://jsperf.com/get-0-vs-0
12 days to next Drupal core point release.
Comment #19
nod_Indeed, I didn't want to change too much stuff in tabledrag so I left it as it was.
I'd say we leave that out from this patch since another issue will be open to deal with selector optimization. But meh, I don't know, that's not what I care about for this patch either way is fine with me.
Comment #20
nod_If you don't mind droplet I'm going for minimum friction, so i'll put it back to NR to keep trying to get reviewers :)
Comment #21
nod_rerererereroll because of #1444160: Drupal.behaviors.dateTime needs a clean-up
Comment #23
nod_well it's a js-only patch, doesn't matter is the testbot is happy or not.
Comment #25
nod_rererererereroll
Comment #26
droplet CreditAttribution: droplet commentedcan I make it as RTBC ?
Comment #27
nod_a rerererererereroll was needed. I'd say it's RTBC but since I made the patch… might want to try getting someone to review it, I've had no luck.
Comment #28
cosmicdreams CreditAttribution: cosmicdreams commentedfound some more instances of this issue
in ajax.js line 377,
was:
$('.throbber', this.progress.element).after('<div class="message">' + this.progress.message + '</div>');
new:
this.progress.element.find('.throbber').after('<div class="message">' + this.progress.message + '</div>');
in states.js line 530,
was:
$('> legend a', e.target).click();
new:
$(e.target).find$('> legend a').click();
in tabledrag.js, line 242,
was:
var field = $('.' + group, row);
new:
var field = $(row).find('.' + group);
in block.js, line 95,
was:
if ($('option[value=' + regionName + ']', regionField).length == 0) {
new:
if ($(regionField).find('option[value=' + regionName + ']').length == 0) {
and line 149,
was:
$('tr.region-message', table).each(function () {
new:
$(table).find('tr.region-message').each(function () {
in comment.js, line 12:
was:
return Drupal.checkPlain($('.form-item-comment input:checked', context).next('label').text());
new:
return Drupal.checkPlain($(context).find('.form-item-comment input:checked').next('label').text());
in field_ui.js, many changes:
(see patch)
in menu.js, line 5:
was:
$('fieldset.menu-link-form', context).drupalSetSummary(function (context) {
new:
$(context).find('fieldset.menu-link-form').drupalSetSummary(function (context) {
line 6:
was:
if ($('.form-item-menu-enabled input', context).is(':checked')) {
new:
if ($(context).find('.form-item-menu-enabled input').is(':checked')) {
line 7:
was:
return Drupal.checkPlain($('.form-item-menu-link-title input', context).val());
new:
return Drupal.checkPlain($(context).find('.form-item-menu-link-title input').val());
in user.permissions.js, line 33
was:
$('input[type=checkbox]', this).not('.rid-2, .rid-1').addClass('real-checkbox').each(function () {
new:
$(this).find('input[type=checkbox]').not('.rid-2, .rid-1').addClass('real-checkbox').each(function () {
line 38:
was:
$('input[type=checkbox].rid-2', this)
new:
$(this).find('input[type=checkbox].rid-2')
Comment #29
droplet CreditAttribution: droplet commented#28 updates reviewed. Looks good.
@cosmicdreams,
do you review other parts ? we hope one more more people review this thread :)
Comment #30
cosmicdreams CreditAttribution: cosmicdreams commented@droplet, I do as much as I can.
I'll manually test this patch today if I have time. I'll go change-by-change this evening.
Comment #31
nod_wow, thanks for finding those. You're right it's like I forgot
field_ui.js
I reviewed #28 too and it's all working fine.I've made a few changes to avoid the redundant use of $() on elements that already are jQuery objects.
There is poor man's interdiff too (diff of two patches…) to see what I changed from #28.
man I hope this is the last, so much patches are starting to conflict… If that goes further I'll be splitting this by module.
Comment #33
nod_yeah yeah, sorry bot got the wrong filename.
Comment #34
JohnAlbin#31: core-js-select-with-find-1419968-30.patch queued for re-testing.
Comment #35
nod_ok, reroll. Wasn't too hard but I'd rather wait next month to get this in.
Oh crap I patched againt D7. sorry will upload followup patch.
Comment #37
nod_ok here it is.
Very soon I will be able to give performance measurements of this kind of changes on behaviors attach time.
Comment #38
JohnAlbingit apply core-js-select-with-find-1419968-37.patch
reports:This patch touches a lot of code (by design). I'm unsure how to test it all in the browser. Is eyeball review of the js the only possible method?
Comment #39
nod_yes, you need to make sure that:
$('XXXselector', DOMELEMENT)
if transformed properly in:$(DOMELEMENT).find('XXXselector')
.I've added some variable as well to avoid redundant uses of
$(this)
, i turned it into:$this = $(this);
, and maybe a var or 2 added because it was leaking but that should be it.edit: change is not a browser thing, don't need to test everywhere.
Comment #40
suntrailing whitespace
Should be:
// @todo Explain what is actually wrong.
The current todo doesn't actually explain what is wrong. Also make sure that the comment wraps at 80 chars. If the todo exceeds 80 chars, indent subsequent lines by two spaces (after the leading //).
Trailing white-space.
---
The manual testing that has been performed already is sufficient for D8, so this patch can land whenever it is ready.
The backport for D7 will likely require more manual testing. However, once it is in D8, we'll get some additional manual testing for free.
Comment #41
nod_Here it is. The comment is not really relevant, it's a comment on the code of this particular part, it uses the elements differently than the rest of the code, it's not using
.get(0)
at the same time. it relies on jquery whereas the rest of the code default to using DOM elements.It's not really important and outside of the scope of this changes. Anyway tabledrag will be rewritten before D8 ships so that'll be taken care of then :)
Comment #42
JohnAlbinFrom the patch:
Which occurs in context here:
Shouldn't you create the
var $context = $(context);
variable outside the for loop? It looks like we're re-creating the same jQuery object on each loop pass.There are other places where we could optimize the code by creating a temporary variable to hold a jQuery object. Should I mention all of them? or just leave it for later optimizations since its out of scope?
I'm only about 1/4 of the way through this review (I'm looking at tabledrag at the moment). I've applied the patch and I'm reviewing the changes, line by line, in context of the actual file. I'll post back when I'm all the way though my review. (Dinner time!)
Comment #43
nod_You're right, $context should be outside of the loop.
I think we should decide if this patch can go in as-is and fix the rest in followup issues otherwise nobody else will want to review this monster again (ask xjm about the #states patch :). On the other end I don't mind adding fixes to this one since the other big conflicting patches are already committed.
When I rerolled I saw a few things I overlooked before but didn't change them. I think it'll be much more efficient to separate by file/module to catch the details. By all means, take notes on what you see, it'll be useful whatever happens.
I guess it all comes down to a core process which I'm not familiar with what to do with big patches where you miss things but it doesn't break the system.
And by the way, thanks a lot for reviewing it :)
Comment #44
JohnAlbinand
Couldn't
$previousRow = $(previousRow).prev('tr').eq(0);
be re-written as$previousRow = $previousRow.prev('tr').eq(0);
?Same issue with
$nextRow = $(nextRow).next('tr').eq(0);
.We don't need to set
previousRow = $previousRow;
in this bit of code becausepreviousRow
isn't used anywhere in or after this bit of code in the Drupal.tableDrag.prototype.updateField function.Is
form
passed as a jQuery object? I can't tell from the code in preview.js. Ditto withform
in bartik/color/preview.js.Shouldn't
$row
be defined like this?var $row = $trigger.closest('tr').eq(0);
That would match the pattern you set earlier.If you address my comments in #42 and #44 and roll a new patch (if needed), I can do an interdiff and mark this RTBC. This is a BIG patch and I went through it with a fine grained comb.
Comment #45
nod_Fixed the $(context) in machine_name.js
Fixed the issues of tabledrag.js
For form in preview, it's a jQuery object, have a look at modules/color/color.js:68 and form is defined at line 12 of the same file.
and for $row, .closest will always return one result, before it was using parents(), it was a mess and it needed the .get(0) to work properly.
Thanks so much for the review. I'll be sure to let you know how the changes affected perfs :D
(edit) ok did some checks on the performance side, some pages takes a bit more time to init on FF, everything's faster in chrome, don't know about IE yet. overall that's positive a couple of page got the time to go wayyyy down. I'll go back working on my reporting tools to have something more meaningful :p
Comment #46
droplet CreditAttribution: droplet commentedIf i'm not wrong...
@John suggest =
Patch #41 is right. That restart to get the object again.
also affect this condition, seems not correct??
Comment #47
nod_droplet is right, I forgot why it was this way but that's indeed the reason why I didn't change it in my previous patches.
reroll of #41 the only change is $context = $(context) is out of the loop for machine_name.
I'm glad it's explained somewhere at least. I want to refactor tabledrag for D8 anyway so we can reduce the huge code debt of this script.
Comment #48
JohnAlbinok. I reviewed the changes in #47 vs. my previous line-by-line review of #41, and all my comments in #42 and #44 have been addressed.
Nice work!
Comment #49
droplet CreditAttribution: droplet commentedAnyway, why it needs eq(0) ??
My suggestion to tableDrag part.
Comment #50
nod_Yeah we should be able to get rid of them. This is not a $(selector, context) into $(context).find('selector') so I'd rather make this change in an other issue, this patch is finally RTBC thanks to JohnAlbin!
We're going to need an "Optimize selectors" issue at some point as well.
Comment #51
JohnAlbinNod_ and droplet, yep, can one of you create a follow-up issue to work on the tabledrag inefficiencies? We can move the patch from #49 to that issue.
Unless others see problems in patch #47 that could break things, it remains RTBC.
Note to committers: Ignore the patch in #49 (it will be a follow-up). The patch in #47 is RTBC. I reviewed it in comments #42, #44 and #48.
Comment #52
catchOK I went to commit this but it no longer applies:
Not sure which patch broke it, but could we get a quick re-roll?
Comment #53
tim.plunkettPatch failure due to #331893: Add colouring (and description) to password checker.
Comment #54
droplet CreditAttribution: droplet commentedHere is the new changes:
Remark for follow up issue:
user.js
Needs a clean-up
Comment #55
catchThanks folks. Committed and pushed to 8.x, moving back to 7.x for backport. We should also add a note in coding standards if there's not one already.
Comment #56
cosmicdreams CreditAttribution: cosmicdreams commentedFollow up created here #1524414: Rewrite tabledrag.js to use jQuery UI
Comment #57
nod_I'd like to hold off on the D7 backport. IE has a weird behavior performance-wise, it's slower overall.
I have a few things planned for D8 that will address that but it'll be too much of a change to be able to backport to D7.
I haven't had time to look exactly why perf are hit like that, tabledrag is definitely one of the reason but that can't be all there is to it.
Comment #58
droplet CreditAttribution: droplet commentedDo you mean IE is slower with this patch ?
It's a big patch. Now is the good time to backport it.
Comment #59
nod_yes, that's what I mean.
If we don't have time to address the perf issue this should not go in D7 and right now I have no time to dig into it. The good patch that needs to get in before and that has a lot of impact (in a good way) is #1428524: Replace all $.each() with filtered for loop.
Comment #60
droplet CreditAttribution: droplet commentedmaybe this issue: #1294584: jQuery performance fix (IE7/8)
Comment #61
tim.plunkettThis was totally busted. Not sure how we/I didn't catch this.
s/contextual-links-wrapper/contextual
Comment #63
tim.plunkett.contextual-links-wrapper used to be the class name, until #1216978: Clean up the CSS for Contextual module. And it IS still the class name in D7.
So that might explain why the wires got crossed here.
Reupping for the bot.
Comment #64
aschiwi CreditAttribution: aschiwi commentedPatch in #61 applies cleanly and works for me.
Comment #65
aschiwi CreditAttribution: aschiwi commentedComment #66
nod_It's already in 8, this is the backport.
Comment #67
tim.plunkettNo, the patch committed to D8 was wrong, and #61/63 (same patch just for the testbot) fix it.
http://drupalcode.org/project/drupal.git/blob/refs/heads/8.x:/core/modul...
There is no div.contextual-links-wrapper in D8.
Comment #68
attiks CreditAttribution: attiks commentedTestSwarm just complained, reason is "find$" instead of "find" in core/misc/states.js 530
Comment #69
cosmicdreams CreditAttribution: cosmicdreams commentedso.....
Which patch needs to be fixed (please reference comment number)
Comment #70
attiks CreditAttribution: attiks commented#53 contains the error
Comment #71
droplet CreditAttribution: droplet commentedComment #72
JamesK CreditAttribution: JamesK commentedTested #71. Looks good.
Comment #73
catchThanks for the follow-up. Committed/push to 8.x, moving to 7.x again.
Comment #74
nod_I'll leave this issue postponed for a while. There is a performance regression in older IEs with this new patch. To compensate for it the code needs to be cleaned up and that's probably way more changes than what is backportable to D7.
I don't want to mess around too much with D7 JS. If nobody is up for better perf testing and fixing it i'll just close this in a while.
Comment #75
sunI actually think that this change, while being a minor performance improvement, is not really worth to backport.
There's simply too much functionality that would have to be tested in-depth, and we'd have to be 100% sure that every single detail works like before, in order to not break hundreds of thousands of Drupal 7 production sites.
Consequently, I think we can spend our time much better with cleaning up JS in D8 and making that some true awesomesauce. Hence, moving back to D8 and fixed.
Comment #76
nod_agreed, thanks.
Comment #77.0
(not verified) CreditAttribution: commentedtypo