[wp-trac] [WordPress Trac] #25112: In large menus javascript freezes the browser and you cannot work with the menu.

[WordPress Trac]

#25112: In large menus javascript freezes the browser and you cannot work with the
menu.
-------------------------------------------------+------------------
 Reporter:  tsdexter                             |       Owner:
     Type:  enhancement                          |      Status:  new
 Priority:  normal                               |   Milestone:  3.7
Component:  Menus                                |     Version:  3.6
 Severity:  major                                |  Resolution:
 Keywords:  2nd-opinion has-patch needs-testing  |
-------------------------------------------------+------------------

Comment (by atimmer):

 The way to profile this is replacing line 1169 with the following:

 {{{
 $(document).ready(function(){
   console.profile( 'menus' );
   console.time( 'menus' );
   wpNavMenu.init();
   console.timeEnd( 'menus' );
   console.profileEnd( 'menus' )
 });
 }}}

 Open the console, then reload the page. The time() is needed in chrome
 because chrome doesn't show total time of a profile. Do this with a
 unminified version of jquery or you won't be able to make any sense out of
 it.

 I hope someone can confirm my findings.

 As for my patch.

 1.
 closest() is exactly the same as parents() functionality wise, the
 difference being how they traverse the DOM tree. see
 [http://api.jquery.com/closest/]. closest() is significantly more
 performant in this case because there is a huge DOM tree and the parent
 element is very 'close' to the element we begin searching from.

 I guess closest() would be slower if we expect to find no match, but in
 our case we know we always get a match.

 2.
 show() and hide(), I didn't know this before I began profiling but what
 happens when you call show() or hide() (they both map to the same function
 internally in jQuery) jquery first has to figure out to which css display
 value it needs to set the element. To do that it queries the current value
 of the display property.

 Somehow that take a few milliseconds, I don't know why but it is the case
 which you see when you profile. The problem comes when we have 7
 show/hide's in our code and it is a loop that loops through every menu
 item on the page. Even if you have a very performant browser that can do
 it in 1ms you get 1*7*[menu-items].

 3.
 $( '#menu-to-edit' ).children( '.menu-item-depth-0' ) instead of $(
 '.menu-item-depth-0' ). The second thing searches the whole DOM for a
 class, the first thing for an ID (which is fast, because ID's are unique)
 and then searches inside the element for the class.

 The reason why these things matter is because they are in a loop that
 loops through all menu item's, a difference of 1ms means a difference of
 1000ms with 1000 menu items.

 Replying to [comment:13 nacin]:
 > I don't think 3000ms (versus 400ms) should mean that the browser
 suddenly crashes. Are we just firing this function too often? Would
 debouncing help?
 3000ms -> 400ms is the patch applied to 170 menu items. I first tried with
 1000 menu item's and that crashed my browser.

 Replying to [comment:11 tsdexter]:
 > In that case it's definitely a bug then because it crashes the latest
 version of chrome every single time you load the page on a brand 2013 27"
 iMac without fail. (on a 1011 item menu)
 >
 > I'm aware thats a large menu but it's necessary as it's used for the
 mobile drilldown menu so it goes many levels deep and long to access the
 whole site and I'm sure other people have similarly long menus as well.
 Can you test my patch and check if the browser still crashes?

 Btw. I think we won't get to levels where 1000 menu item's will be very
 performant considering refreshAdvancedAccessibility is called on every
 drag of a menu item. But at least we can do better than current 3.6.

--
Ticket URL: <http://core.trac.wordpress.org/ticket/25112#comment:14>
WordPress Trac <http://core.trac.wordpress.org/>
WordPress blogging software