Currently the logic for breakpoints triggers a menu rebuild for every enabled theme and then for each of those themes for every enabled breakpoint. This will then call menu_rebuild on line 102 under the breakpoints_themes_enabled() function which itself gets triggered from breakpoints_flush_caches.

Remove the menu_rebuild just to test caused my drush cc all to perform significantly faster by about 1min and 5 seconds.

We first caught this performance issue when using XHProf and then Xdebug + Cachegrind and noticed breakpoint_flush_caches was at the top of the stack.

CommentFileSizeAuthor
#13 2415363-breakpoints-menu_rebuild-13.patch2.21 KBhefox
#8 breakpoints-2415363.patch1.95 KBquicksketch
#1 performance_issue_with-2378449-1.patch1.32 KBsylus
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

sylus’s picture

sylus CreditAttribution: sylus commented
FileSize
performance_issue_with-2378449-1.patch1.32 KB

I have added a quick patch to the hook_flush_all_caches implementation to disable the menu_rebuild till can find a better way of calling the function.

Results were as follows:

a) Before Patch

$ time drush cc all

real 2m45.362s
user 1m47.015s
sys 0m13.277s

b) After Patch

$ time drush cc all

real 0m55.370s
user 0m38.866s
sys 0m4.568s

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Sylus, nice work finding this problem and getting a solution out! Really appreciate this one! :)

p-andrei’s picture

p-andrei CreditAttribution: p-andrei commented

Hey module maintainers! Any plans to include this patch in next releases? Its extremely annoying to wait for so long on each clear cache.

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

Use variable_set('menu_rebuild_needed', TRUE) instead of calling menu_rebuild()

Dave Reid’s picture

Dave Reid
he/him
Primary language English
Location Nebraska USA
CreditAttribution: Dave Reid commented

I think this would be resolved with #2415363: Replace calls with menu_rebuild with variable_set('menu_rebuild_needed')?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

Confirming this issue. We're seeing Breakpoints module responsible for up to 30 seconds of additional processing time due to rebuilding the menu once per enabled theme. I think Dave Reid's approach is less disruptive and solves the issue in the right way. The patch in this issue would still rebuild the menu twice, whereas #2415363: Replace calls with menu_rebuild with variable_set('menu_rebuild_needed') keeps the menu only built a single time during cache clears.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

This patch should be applied in addition to Dave Reid's patch. See my explanation in #2415363-4: Replace calls with menu_rebuild with variable_set('menu_rebuild_needed'). I do think the parameter to breakpoints_themes_enabled() could use a better name than $force however, how about $rebuild_menu?

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented
Status: Active » Needs review
FileSize
breakpoints-2415363.patch1.95 KB

Because this issue overlaps with Dave Reid's patch from the other issue, I've combined them together if we'd prefer to just apply one patch. And this renames the parameter to $rebuild_menu.

joseph.olstad’s picture

joseph.olstad
Primary language French
CreditAttribution: joseph.olstad commented

Hi quicksketch, I was only able to apply your patch by it'self, neither Sylus's patch, nor Dave Reid's patch apply once yours is applied. Have these patches been rolled into one with your patch?

So to avoid redundant calls to menu_rebuild we just need breakpoints 7.x-1.x and your patch from comment #8 ? or is there another patch necessary?

Here's how I applied your patch:

git clone --branch 7.x-1.x http://git.drupal.org/project/breakpoints.git
cd breakpoints
wget http://drupal.org/files/issues/breakpoints-2415363.patch
patch -p1 < breakpoints-2415363.patch
patching file breakpoints.module

this works, but if I try applying Dave Reid's patch on top or Sylus's patch on top there's reject messages.

Please clarify then I'll proceed with testing your patch.

quicksketch’s picture

quicksketch
he/him
CreditAttribution: quicksketch commented

> Hi quicksketch, I was only able to apply your patch by it'self, neither Sylus's patch, nor Dave Reid's patch apply once yours is applied. Have these patches been rolled into one with your patch?

Sorry I wasn't clear! Yes, the patch in #8 is a combined patch that includes both Dave Reid's patch and Sylus's patch.

sylus’s picture

sylus CreditAttribution: sylus commented
Status: Needs review » Reviewed & tested by the community

Thanks @quicksketch all of your improvements are more then reasonable and agreed about renaming force to menu_rebuild.

This patch worked for me and still fixed the performance issue :)

The last submitted patch, 1: performance_issue_with-2378449-1.patch, failed testing.

hefox’s picture

hefox CreditAttribution: hefox commented
Status: Reviewed & tested by the community » Needs review
FileSize
2415363-breakpoints-menu_rebuild-13.patch2.21 KB

quick 3 line change to only set the variable in breakpoints_themes_enabled if TRUE and something has actually changed.

Unfortunately back to needs review

RavindraSingh’s picture

RavindraSingh CreditAttribution: RavindraSingh as a volunteer and at Srijan | A Material+ Company commented
Status: Needs review » Reviewed & tested by the community

making #13 RTBC, as i have tested this.

Ravindras-MacBook-Pro:breakpoints ravindrasingh$ time drush cc all
'all' cache was cleared.                                                                                   [success]

real	0m20.532s
user	0m5.733s
sys	0m0.907s
Stefan Lehmann’s picture

Stefan Lehmann CreditAttribution: Stefan Lehmann commented

Also was able to apply patch and it reduced the cache clear time significantly. We have a lot of websites running with the picture module which has a dependency to this module and it would be really good to see this patch go into the major version release.

  • attiks committed 854dc71 on 7.x-1.x authored by hefox 
    Issue #2378449 by quicksketch, sylus, hefox, joseph.olstad, Dave Reid:...
attiks’s picture

attiks CreditAttribution: attiks at Attiks commented
Status: Reviewed & tested by the community » Fixed

Thanks, I somehow missed this one, committed and new release created

Status: Fixed » Closed (fixed)

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