Problem/Motivation

The search links are populated whenever the admin_toolbar_search.js file is loaded.

I timed the $('.toolbar-tray a[data-drupal-link-system-path]').each(function () {}); loop function by using the js .time() function and found out that it takes approx 40-45ms to complete. That execution time can be defferred to a later stage, when the search bar is actually displayed. In this case we can boost our loading time performance and not block the thread with this function.

Proposed resolution

I suggest we add helper function for that and call it only when necessary - when the search bar is visible or when it's expanded.

CommentFileSizeAuthor
#6 3125253-toolbar-search-performance-improvement-6-interdiff.txt4.3 KBsasanikolic
#6 3125253-toolbar-search-performance-improvement-6.patch2.18 KBsasanikolic
#2 3125253-toolbar-search-performance-improvement.patch2.12 KBsasanikolic
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sasanikolic created an issue. See original summary.

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commented
Status: Active » Needs review
FileSize
3125253-toolbar-search-performance-improvement.patch2.12 KB

Here is the proposed patch with the improvement.

miro_dietiker’s picture

miro_dietiker
Location Switzerland
CreditAttribution: miro_dietiker at MD Systems GmbH commented 
+++ b/js/admin_toolbar_search.js
@@ -155,6 +145,26 @@
+            $self.links.push({

From reading, i think this currently means that if you enter the search box twice, you will push each URL twice, and more...

Also it means each time you enter, the 40ms are spent again.

We should limit this to a single init execution only.

Or am i maybe wrong?

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commented 
+++ b/js/admin_toolbar_search.js
@@ -155,6 +145,26 @@
+      if ($self.links.length === 0) {

@miro_dietiker That's why I added this check here. If it's already populated, the code will not execute again.

Berdir’s picture

Berdir
Primary language German
Location Switzerland
CreditAttribution: Berdir at MD Systems GmbH commented 
+++ b/js/admin_toolbar_search.js
@@ -155,6 +145,26 @@
+            $self.links.push({
+              'value': $(this).attr('href'),
+              'label': label + ' ' + $(this).attr('href'),
+              'labelRaw': label

can we get the attribute directly from this instead of going through jquery again? that could remove quite a bit of overhead I imagine?

sasanikolic’s picture

sasanikolic CreditAttribution: sasanikolic at MD Systems GmbH commented
FileSize
3125253-toolbar-search-performance-improvement-6.patch2.18 KB
3125253-toolbar-search-performance-improvement-6-interdiff.txt4.3 KB

I noticed the duplicated admin_toolbar_search.js and opened a followup for that: .

Moved the code to the right file now and updated value and label to get the href using javascript instead of jquery.

adriancid’s picture

adriancid
Primary language Spanish
Location Montreal, Canada
CreditAttribution: adriancid at Drupiter commented
Status: Needs review » Fixed

Thanks

Status: Fixed » Closed (fixed)

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