WordPress.org

Make WordPress Core

Opened 6 years ago

Closed 5 years ago

Last modified 5 years ago

#10021 closed defect (bug) (fixed)

Unresponsive script errors on widgets screen

Reported by: Denis-de-Bernardy Owned by: azaozz
Milestone: 2.9 Priority: normal
Severity: normal Version: 2.8
Component: JavaScript Keywords:
Focuses: Cc:

Description

I'm getting unresponsive script errors in Camino on the widgets screen, when multitudes of widgets and widget fields are present.

I've nailed this down to an optimization that is possible in the various jQuery functions. Specifically, the likes of find(".class") can be changed to find("tag.class") for the same functionality *and* a faster selector.

Attached patch does enough for the error message to go away while keeping the functionality intact.

Attachments (8)

10021.diff (9.8 KB) - added by Denis-de-Bernardy 6 years ago.
logger.diff (6.6 KB) - added by Denis-de-Bernardy 6 years ago.
10021.2.diff (17.7 KB) - added by Denis-de-Bernardy 6 years ago.
not finished yet…
10021.3.diff (17.4 KB) - added by Denis-de-Bernardy 6 years ago.
widgets.diff (28.6 KB) - added by Denis-de-Bernardy 6 years ago.
10021.4.diff (723 bytes) - added by Denis-de-Bernardy 6 years ago.
10021.5.diff (1.5 KB) - added by Denis-de-Bernardy 6 years ago.
also optimize the sortables
10021.6.diff (648 bytes) - added by Denis-de-Bernardy 6 years ago.
fix parse error in the script

Download all attachments as: .zip

Change History (43)

@Denis-de-Bernardy6 years ago

comment:1 @Denis-de-Bernardy6 years ago

  • Resolution set to wontfix
  • Status changed from new to closed

per IRC discussion

comment:2 @Denis-de-Bernardy6 years ago

  • Milestone 2.8 deleted

comment:3 @Denis-de-Bernardy6 years ago

  • Milestone set to 2.8.4
  • Priority changed from normal to high
  • Resolution wontfix deleted
  • Severity changed from normal to major
  • Status changed from closed to reopened

Soo.. I added a logger in there (patch to make it appear is attached). And the breakdown for my screen is the following:

0ms -- adminMenu.init()
19ms -- $('#adminmenu div.wp-menu-toggle').each
5ms -- this.favorites();
4ms -- $('a.separator').click
1ms -- $('body').hasClass('folded')
8ms -- this.restoreMenuState();
1ms -- end adminMenu.init()
0ms -- columns.init()
4ms -- end columns.init()
0ms -- commons.ready()
4ms -- $('.fade').animate
7ms -- $('div.wrap h2 ~ div.updated, div.wrap h2 ~ div.error').addClass('below-h2');
12ms -- $('div.updated, div.error').not('.below-h2').insertAfter('div.wrap h2:first');
5ms -- $('#doaction, #doaction2').click
1ms -- $('#show-settings-link').click
1ms -- $('#contextual-help-link').click
6ms -- $('#contextual-help-link-wrap, #screen-options-link-wrap').show();
743ms -- $( 'table:visible tbody .check-column :checkbox' ).click
227ms -- $( 'thead :checkbox, tfoot :checkbox' ).click
3ms -- $('#default-password-nag-no').click
1ms -- end commons.ready()
1ms -- turbo.ready()
7ms -- end turbo.ready()
1ms -- widgets.init()
1ms -- $('body').hasClass('widgets_access')
7ms -- $('#widgets-right div.sidebar-name').click
6ms -- $('#widgets-left div.sidebar-name').click
45ms -- $('#widgets-right .widget, #wp_inactive_widgets .widget').each
35ms -- this.addEvents();
4ms -- $('.widget-error').parents('.widget').find('a.widget-action').click();
8ms -- $('#available-widgets').droppable
31ms -- $('#widget-list .widget').draggable
1092ms -- $('.widgets-sortables').sortable
10ms -- wpWidgets.resize();
367ms -- wpWidgets.fixLabels();
1ms -- end widgets.init()

@Denis-de-Bernardy6 years ago

comment:4 @Denis-de-Bernardy6 years ago

the logger patch is against today's 2.8 branch

comment:5 @Denis-de-Bernardy6 years ago

  • Keywords needs-patch added; has-patch tested removed

comment:6 @Denis-de-Bernardy6 years ago

oh, the latter is for a widgets screen with lots of checkboxes, btw.

comment:7 @Denis-de-Bernardy6 years ago

Soo... making good progress...

0ms -- adminMenu.init()
12ms -- $('#adminmenu div.wp-menu-toggle').each
5ms -- this.favorites();
3ms -- $('a.separator').click
1ms -- $('body').hasClass('folded')
5ms -- this.restoreMenuState();
0ms -- end adminMenu.init()
1ms -- columns.init()
3ms -- end columns.init()
0ms -- commons.ready()
3ms -- $('.fade').animate
5ms -- $('div.wrap h2 ~ div.updated, div.wrap h2 ~ div.error').addClass('below-h2');
9ms -- $('div.updated, div.error').not('.below-h2').insertAfter('div.wrap h2:first'); --- potential optimization left
3ms -- $('#doaction, #doaction2').click
1ms -- $('#show-settings-link').click
0ms -- $('#contextual-help-link').click
7ms -- $('#contextual-help-link-wrap, #screen-options-link-wrap').show();
8ms -- $( 'table:visible tbody .check-column :checkbox' ).click
4ms -- $( 'thead :checkbox, tfoot :checkbox' ).click
2ms -- $('#default-password-nag-no').click
1ms -- end commons.ready()
0ms -- turbo.ready()
6ms -- end turbo.ready()
1ms -- widgets.init()
1ms -- $('body').hasClass('widgets_access')
8ms -- $('#widgets-right div.sidebar-name').click
5ms -- $('#widgets-left div.sidebar-name').click
106ms -- $('#widgets-right .widget, #wp_inactive_widgets .widget').each
46ms -- this.addEvents();
4ms -- $('.widget-error').parents('.widget').find('a.widget-action').click();
4ms -- $('#available-widgets').droppable
29ms -- $('#widget-list .widget').draggable
1071ms -- $('.widgets-sortables').sortable
9ms -- wpWidgets.resize();
529ms -- wpWidgets.fixLabels();
1ms -- end widgets.init()

comment:8 @aaroncampbell6 years ago

Just adding a quick note here for others like me that are a little out of the loop on jQuery's inner workings.

My numbers were pretty different from the ones noted above because I had far fewer widgets. However, $( 'table:visible tbody .check-column :checkbox' ).click was taking 40-45 ms for me. I added some specificity $( 'table:visible tbody .check-column input:checkbox' ).click and it sped up to 10-11 ms.

Denis then explained that jQuery processes selectors from right to left. This means that in it's original form it actually gets all checkboxes then throws out the ones that don't have an ancestor with the check-column class, etc. He offered an even more efficient alternative $('table:visible').children('tbody').children('tr.check-column').find('input:checkbox') which processes completely differently by first getting visible tables, then getting their bodies, ... and eventually getting the checkboxes. This method reduced the processing time to 2-3 ms for me.

That's 40-45ms -> 10-11ms -> 2-3ms, which is quite an improvement. It seems like this could be applied to any jQuery selectors that have common elements toward the right side of the selector, especially if the left side of the selector reduces the total amount of nodes that need to be processed.

comment:9 @Denis-de-Bernardy6 years ago

just adding to the previous, the correct selector for that one actually is:

$('table:visible').children('tbody').children().children('.check-column').find('input:checkbox');

(the one aaron mentioned results in an empty array, and it's my bad, not his.) using that brings things to 10ms, from 750ms, on my widgets screen.

@Denis-de-Bernardy6 years ago

not finished yet...

comment:10 @aaroncampbell6 years ago

  • Cc aaroncampbell added

Just a quick note, the latest diff looks like it has the optimizations as well as the logging. I'm not sure if that was intentional.

A huge thanks for this by the way, and if you post when you're done, I'd be happy to test.

@Denis-de-Bernardy6 years ago

comment:11 @Denis-de-Bernardy6 years ago

  • Keywords has-patch needs-testing added; needs-patch removed

The figures with the patch applied...

Camino 2.0:

0ms -- adminMenu.init()
14ms -- $('#adminmenu div.wp-menu-toggle').each
4ms -- this.favorites();
4ms -- $('a.separator').click
1ms -- $('body').hasClass('folded')
5ms -- this.restoreMenuState();
0ms -- end adminMenu.init()
1ms -- columns.init()
3ms -- end columns.init()
0ms -- commons.ready()
3ms -- $('.fade').animate
6ms -- $('div.wrap h2 ~ div.updated, div.wrap h2 ~ div.error').addClass('below-h2');
9ms -- $('div.updated, div.error').not('.below-h2').insertAfter('div.wrap h2:first');
4ms -- $('#doaction, #doaction2').click
1ms -- $('#show-settings-link').click
1ms -- $('#contextual-help-link').click
6ms -- $('#contextual-help-link-wrap, #screen-options-link-wrap').show();
9ms -- $( 'table:visible tbody .check-column :checkbox' ).click
5ms -- $( 'thead :checkbox, tfoot :checkbox' ).click
2ms -- $('#default-password-nag-no').click
1ms -- end commons.ready()
0ms -- turbo.ready()
6ms -- end turbo.ready()
1ms -- widgets.init()
1ms -- $('body').hasClass('widgets_access')
4ms -- $('#widgets-right div.sidebar-name').click
3ms -- $('#widgets-left div.sidebar-name').click
2ms -- $('#widgets-right .widget, #wp_inactive_widgets .widget').each
48ms -- this.addEvents();
4ms -- $('.widget-error').parents('.widget').find('a.widget-action').click();
4ms -- $('#available-widgets').droppable
27ms -- $('#widget-list .widget').draggable
169ms -- $('.widgets-sortables').sortable
10ms -- wpWidgets.resize();
11ms -- wpWidgets.fixLabels();
0ms -- end widgets.init()

Safari:

0ms -- adminMenu.init()
10ms -- $('#adminmenu div.wp-menu-toggle').each
7ms -- this.favorites();
8ms -- $('a.separator').click
3ms -- $('body').hasClass('folded')
4ms -- this.restoreMenuState();
2ms -- end adminMenu.init()
0ms -- columns.init()
3ms -- end columns.init()
1ms -- commons.ready()
3ms -- $('.fade').animate
3ms -- $('div.wrap h2 ~ div.updated, div.wrap h2 ~ div.error').addClass('below-h2');
5ms -- $('div.updated, div.error').not('.below-h2').insertAfter('div.wrap h2:first');
2ms -- $('#doaction, #doaction2').click
0ms -- $('#show-settings-link').click
1ms -- $('#contextual-help-link').click
10ms -- $('#contextual-help-link-wrap, #screen-options-link-wrap').show();
5ms -- $( 'table:visible tbody .check-column :checkbox' ).click
3ms -- $( 'thead :checkbox, tfoot :checkbox' ).click
1ms -- $('#default-password-nag-no').click
2ms -- end commons.ready()
1ms -- turbo.ready()
8ms -- end turbo.ready()
1ms -- widgets.init()
2ms -- $('body').hasClass('widgets_access')
2ms -- $('#widgets-right div.sidebar-name').click
1ms -- $('#widgets-left div.sidebar-name').click
2ms -- $('#widgets-right .widget, #wp_inactive_widgets .widget').each
9ms -- this.addEvents();
1ms -- $('.widget-error').parents('.widget').find('a.widget-action').click();
4ms -- $('#available-widgets').droppable
7ms -- $('#widget-list .widget').draggable
89ms -- $('.widgets-sortables').sortable
5ms -- wpWidgets.resize();
3ms -- wpWidgets.fixLabels();
1ms -- end widgets.init()

comment:12 @Denis-de-Bernardy6 years ago

I'll upload a separate patch without the log junk after it gets more testing.

comment:13 @Denis-de-Bernardy6 years ago

the jQuery bug I opened for reference:

http://dev.jqueryui.com/ticket/4759

comment:14 @Denis-de-Bernardy6 years ago

Widgets screen, with patch applied, is still fully functional on my end. I can add, move, save, close, remove, drop into inactive widgets, move from a sidebar to the next... and I can still remove columns from the editor's screen.

tested in camino 2 and safari 4

it needs FF and IE testing now. I'll get an IE user onto it this WE.

comment:15 @Denis-de-Bernardy6 years ago

these two are with an even larger amount of widgets and checkboxes, on FF 3.5.2 and IE 8 respectively:

http://pastebin.com/m7bf7bf9d

http://pastebin.com/m694817c3

this is *after* applying the patch. so, still, a few optimizations to go from the looks of it.

comment:16 @Denis-de-Bernardy6 years ago

my own stats with the very same data as in the previous comment:

Camino - http://pastebin.com/m6c719f77

Safari - http://pastebin.com/mba59f13

comment:17 follow-up: @filosofo6 years ago

You're re-arranging deck chairs on the Titanic. jQuery is buggy and inefficient. The UI plugins have been unreliable for years. Every new jQuery release breaks WordPress plugins and themes using older versions. A better solution would be to dump it.

Think of the benefits of having our own WordPress admin JavaScript libraries.

  • It could have a reliable, stable API across development cycles
  • We could tailor it to meet specific needs. For example here, instead of having to attach event listeners to zillions of widget elements, we could use event delegation.
  • We could use progressive enhancement techniques to make the admin more accessible.

comment:18 @Denis-de-Bernardy6 years ago

As much as I agree on principle, the immediate need is to ensure that themes with tons of sidebars (I know I'm not the only one) doesn't crash browsers...

comment:19 @filosofo6 years ago

I understand the short-term need, but I hope to stir up interest in a long-term solution.

comment:20 @Denis-de-Bernardy6 years ago

here's another interesting one... show() and :hidden are extremely slow on slow rendering engines, e.g. Opera 9 with a 400kb widgets page gets:

6ms -- $('#contextual-help-link-wrap')
6071ms -- $('#contextual-help-link-wrap:hidden')

the next calls are very fast, until a new class gets added.

similarly, adding an arbitrary ID to the page will render the next selector that users "#whatever" extremely slow in some engines...

comment:21 @Denis-de-Bernardy6 years ago

by the same token, this:

$( 'table:visible tbody .check-column :checkbox' ).click(...)

is much slower than merely:

$( 'tbody .check-column :checkbox' ).click(...)

comment:22 @aaroncampbell6 years ago

  • Keywords tested added; needs-testing removed

I finally got a chance to fully test in IE 8, FF 3.5.2, Chrome 2, Safari 4, Opera 9.64. In every browser I'm able to drag widgets to sidebars as well as the inactive area, drag widgets between sidebars, edit and save widgets, remove widgets, etc.

The speed differences are definitely measurable, although I wasn't testing with very many widgets or sidebars.

@Denis-de-Bernardy6 years ago

comment:23 @Denis-de-Bernardy6 years ago

  • Keywords needs-testing added; tested removed

improved version of the patch attached. I'm still looking into avoiding to change the widget ID generation.

comment:24 @azaozz6 years ago

  • Milestone changed from 2.8.5 to 2.9
  • Priority changed from high to normal
  • Severity changed from major to normal

Been testing this for a while. For an average widgets page with two sidebars and about 40 widgets on a lower end PC the speed increase is almost unnoticeable:

 Firefox 3.5  195ms -> 125ms
 Safari 4     120ms -> 92ms
 Opera 9.6    830ms -> 640ms
 IE6          1540ms -> 1130ms

(note how browsers without getElementsByClassName are a lot slower). There's only a barely noticeable speed improvement in IE6, 0.4sec. Of course the numbers change in extreme cases: 12 sidebars and 150 widgets containing tables, etc.

Nevertheless this patch is a good idea and would fix some border cases where the widgets page is huge, 1MB+.

The only problem as Denis mentions is that the current patch modifies all internal widgets HTML IDs making them unusable for plugins and breaking any plugin that uses them at the moment. This is done to speed up copying the widget's title from the title field to the widget's header.

There's also a regression there as the title used to be appended when the widget was saved too. Trying some workarounds for this but may eventually need to put the old code back.

comment:25 @azaozz6 years ago

(In [11837]) Speed up jQuery based scripts, props Denis-de-Bernardy, see #10021

comment:26 @Denis-de-Bernardy6 years ago

Sorry for not having followed this much lately. I spotted an issue in my original code.

basically, not all admin screens (in plugins) use:

<form>
<div class="wrap">
<h2>...

some use this instead:

<div class="wrap">
<form>
<h2>...

I'm attaching the needed patch to what was committed.

@Denis-de-Bernardy6 years ago

@Denis-de-Bernardy6 years ago

also optimize the sortables

comment:27 @Denis-de-Bernardy6 years ago

10021.5.diff also includes an optimization to the sortables. in opera 9.6, it made quite a difference when I tried it (see the jquery bug I submitted further up)

comment:28 @azaozz6 years ago

(In [11858]) More jQuery based scripts optimizations, props Denis-de-Bernardy, see #10021

comment:29 in reply to: ↑ 17 ; follow-up: @azaozz6 years ago

Replying to filosofo:

Think of the benefits of having our own WordPress admin JavaScript libraries...

In principle this sounds good however in practice it would be quite hard to convince plugin and theme authors to learn yet another API. It's hard enough to make them stop bundling jQuery and use the one included in core.

If we switch to our own JS API at least one JS library will still be loaded on every page by a plugin or theme anyways. Also jQuery is getting better, 1.3.3 seems to be coming out soon and has some nice speed improvements.

comment:30 in reply to: ↑ 29 @filosofo6 years ago

Replying to azaozz:

In principle this sounds good however in practice it would be quite hard to convince plugin and theme authors to learn yet another API. It's hard enough to make them stop bundling jQuery and use the one included in core.

The theme and plugin authors can continue to use jQuery if they want, as I'm sure we'd continue bundling it as we do Prototype, for example. But it's not like learning the jQuery API is all that helpful anyways, as it has changed significantly between versions, and we don't have any reason to think the future will be different.

The advantages of a WordPress-built JavaScript API is that it could be truly stable, mainly by focusing on a few things that always need to be done in the WP admin: drag-and-drop, XHR, etc.

If we switch to our own JS API at least one JS library will still be loaded on every page by a plugin or theme anyways.

There's not much we can do to avoid badly-written plugins; they're always going to exist. But the properly-written ones should be loading their libraries only on their own pages, with a few exceptions.

Also jQuery is getting better, 1.3.3 seems to be coming out soon and has some nice speed improvements.

I used to hold out hope for future improvements, too, but jQuery has had plenty of opportunities to address well-known problems and to take advantage of lots of constructive criticism. I see selector speed reports and the like, but they don't mean much when I can turn on Firebug in actual applications and profile, for example, a simple drag-and-drop with tens of thousands of function calls. Or I go open a page on Slashdot and my CPU usage spikes for few seconds while jQuery churns.

@Denis-de-Bernardy6 years ago

fix parse error in the script

comment:31 @Denis-de-Bernardy6 years ago

another patch, to fix the parse error that crept in.

comment:32 @azaozz6 years ago

(In [11870]) Fix error in widgets.js, props Denis-de-Bernardy, see #10021

comment:33 @azaozz6 years ago

So I must have tested it with cached js, no wonder that didn't see any difference. Don't see much improvement now too, maybe few milliseconds on an average widgets screen.

comment:34 @azaozz5 years ago

  • Keywords has-patch needs-testing removed
  • Resolution set to fixed
  • Status changed from reopened to closed

Seems to be working well.

comment:35 @nacin5 years ago

Cross-referencing #11267 for a change in the admin notice animation.

Note: See TracTickets for help on using tickets.