WordPress.org

Make WordPress Core

Opened 9 years ago

Closed 6 years ago

#15650 closed enhancement (worksforme)

Inefficient selectors in common.dev.js

Reported by: GamajoTech Owned by:
Milestone: Priority: normal
Severity: minor Version:
Component: Administration Keywords:
Focuses: javascript, performance Cc:
PR Number:

Description

There are some repeated jQuery selector lookups that could be cached, and incorrect attempts at adding contexts in common.dev.js.

Some minor optimisations that sacrifices a little bit of code readability, for a double speed improvement of caching expensive jQuery selector lookups, and reducing the size of common.js by ~300 bytes (5%) when minifying.

Attachments (2)

common.dev.js.patch (7.0 KB) - added by GamajoTech 9 years ago.
Minor performance optimisations
common.dev.js.2.patch (13.6 KB) - added by GamajoTech 9 years ago.

Download all attachments as: .zip

Change History (13)

@GamajoTech
9 years ago

Minor performance optimisations

#1 follow-up: @scribu
9 years ago

We should make a JavaScript guideline page in the codex with the rule that jQuery collection variables start with a '$'. This is common practice.

In your patch, this would mean $body instead of body etc.

#2 follow-up: @azaozz
9 years ago

These two do the same thing:

$('.wp-menu-toggle', menu)...
menu.find('.wp-menu-toggle')...

They limit the search for the .wp-menu-toggle class only to children of menu. Did some testing few months ago and the first one was slightly faster.

It can also be written as:

$('li.wp-has-submenu', '#adminmenu')...

i.e. find li nodes that have wp-has-submenu class but search only in the node with id = adminmenu.

#3 follow-up: @nacin
9 years ago

  • Keywords needs-refresh added; has-patch needs-testing removed
  • Milestone changed from Awaiting Review to Future Release
  • Type changed from defect (bug) to enhancement

A few of these make sense. But please see azaozz's comments.

Also, we use tabs, not spaces. If you can correct the patch, it would be more readable.

Optimizations like wpHasSubmenu aren't necessary, as it's only used once (referenced twice, but there's an if/else there).

#4 in reply to: ↑ 1 @GamajoTech
9 years ago

Replying to scribu:

We should make a JavaScript guideline page in the codex with the rule that jQuery collection variables start with a '$'. This is common practice.

In your patch, this would mean $body instead of body etc.

I completely agree, but I couldn't find any agreed guidelines about which preference WP code should use.

#5 in reply to: ↑ 2 @GamajoTech
9 years ago

Replying to azaozz:

These two do the same thing:

$('.wp-menu-toggle', menu)...
menu.find('.wp-menu-toggle')...

They limit the search for the .wp-menu-toggle class only to children of menu.

Agreed.

Did some testing few months ago and the first one was slightly faster.

As the first is converted internally to use the second, I find your results interesting. Could you share your test code?

From http://api.jquery.com/jQuery/

Internally, selector context is implemented with the .find() method, so $('span', this) is equivalent to $(this).find('span').

I see no reason not to jump straight to using the find() method for the optimizations I've listed.

It can also be written as:

$('li.wp-has-submenu', '#adminmenu')...

i.e. find li nodes that have wp-has-submenu class but search only in the node with id = adminmenu.

Actually, no it can't. Again from http://api.jquery.com/jQuery/ the context is:

A DOM Element, Document, or jQuery to use as context

A string doesn't cut it; the context would still be "document", and not "#adminmenu".

Further information at http://brandonaaron.net/blog/2009/06/24/understanding-the-context-in-jquery

#6 in reply to: ↑ 3 @GamajoTech
9 years ago

Replying to nacin:

A few of these make sense. But please see azaozz's comments.

Also, we use tabs, not spaces. If you can correct the patch, it would be more readable.

Optimizations like wpHasSubmenu aren't necessary, as it's only used once (referenced twice, but there's an if/else there).

All noted. I'll re-do the patch.

#7 @nacin
9 years ago

We don't enforce or typically use the $body style.

#8 @Denis-de-Bernardy
9 years ago

@GamajoTech: see #10021

#9 @GamajoTech
9 years ago

Switched from spaces to tabs. Also expanded on abbreviated variables (no real reason to keep abbreviations in a .dev.js as the variables get renamed upon minification anyway). Sorted a few spacing issues, but there are still plenty of inconsistencies (waiting on core contributor handbook).

#10 @nacin
6 years ago

  • Component changed from Performance to Administration
  • Focuses javascript performance added

#11 @ocean90
6 years ago

  • Keywords needs-refresh removed
  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

Most things from common.dev.js.2.patch are implemented. Please open new tickets for further improvements.

Note: See TracTickets for help on using tickets.