D7 Improve theme registry build performance by 85%

Link to last D7 patch: #2339447-131: Improve theme registry build performance by 85%

This is the patch from comment #2 above. ***EDIT*** Its from the parent issue, the last one. ***EDIT***

Creds to Quicksketch , donquixote, mikeytown2, joelpittet , and others in parent issue.

***EDIT***We've been using a different version of this patch***EDIT*** (#73 of Quicksketch in parent issue) for over a year now, and a derivative of the 7.x patch was pushed already into 8.x a while back.

PHP 7 is passing... curious why PHP 5.3 and 5.4 are not.

This is the Quicksketch patch 73 from the parent issue renamed for this issue.

We've been using this one with D7 7.44 and D7 7.50
and a long time with previous D7 releases as well.

Patch 73 from the parent issue is the one the wetkit distro has been using ever since it was published by Quicksketch , there's several sites thats been using it for that long.

We've been using it in production since then and have also been using it on the latest D7 releases 7.44 and 7.50 as well.

RTBC patch #6

Latest version of Backdrop is using this patch (based on Drupal 7)
Drupal 8 8.1.x is using this patch as well, same code was committed into D8

Quicksketch is the developer of Backdrop and of this patch, great work and thanks for his generosity and for the others!

This patch is tops on my wishlist for the next D7 release.

I am okay with RTBC, but we are postponed on the newer fix to get into D8 first.

That is the backport policy - sorry.

Fabianx , there is a new backport policy, in this case because of divergence we can work on both the D8 issue and the D7 issue simultaneously.

The D7 changes being in a new issue is part of the new backport policy here #2462101: [policy, no patch] Update backport policy to account for 8.x minor versions and 7.x divergence and
https://www.drupal.org/core/backport-policy#d7

It's very demotivating to see a patch that was written for D7 get patched in D8 and then for god knows what reason the D8 patch , while improving performance by leaps and bounds , needs another patch to make it even faster, meanwhile D7 has the original slow code, from WHAT I'VE read in the backport policy there's nothing saying that several revisions of the D8 improvement MUST be made before D7 gets a single drop of water. Instead of Drupal 7 we should call it DehydRution 7

Drupal 7 is next up to become unsupported , time is important, people need this performance improvement while there's still a reason for it.

Ok, the requested D8 patch
#2720849: D8 improve theme registry build performance
IS ready, so now D7 is readier than ever

This patch is by @donquixote , from comment #124 of the parent issue.

It passed all tests after kicking the testbot a few times on one of them.

We've always used patch #6 (Quicksketch patch 73 from the parent issue)

@donquixote says his patch is slightly faster (see his explanation) (although a whole lot more code).

I don't have a preference except appreciate the shortness of #73, (as long as one of them gets committed our team will be happy.)

testbot passes after the 5.3 test was kicked a second time
set to RTBC, tests will be re-run, may need a few kicks.

It has been a while since I last looked into all this. So I currently can't say much about whether I recommend my approach or not.

But if we talk about my approach it should be the patch from #2339447-131, not #2339447-124 !
Re-uploading it here.

Please look at the comments following #2339447-131 in the other issue.

@donquixote D7-2720853-17-drupal_find_theme_functions-optimize.patch is 0 bytes

Thanks for letting me know.
I hate patches!

#11: There is no divergence in the code base, I am sorry. And please refrain from word games like that.

What about we get quicksketchs short patch #73 in for Drupal 8 and then open a new issue again for the more complex approach by donquixote.

The #73 patch of the parent issue is very easy, clean and easily committable, while my own approach (that got committed) is way more complex and such error prone.

Therefore it neither makes sense for my approach to go into D7 nor makes it sense to not have the easier and better approach go into D8 first.

--

I dislike the backport policy as well (my personal opinion), but as a maintainer I have to adhere to it and it works, because:

But lets say we had committed the quicksketch patch to D7, in 5 years time Drupal 8 then still have the slower approach as that issue got forgotten in some queue somewhere.

Then you update your distribution to Drupal 8 and suddenly you have the same problem that you solved in D7 already again.

Until you find the bug, things are slower than before, so you put in time and effort and finally see that Drupal 8 never got the "good" fix.

You would probably be angry about that regression and not understand how that could have happened.

--

And that is the reason for the backport policy. To spare _you_ work, time and effort.

So it is in your best interest as well.

Postponing again on grounds of RTBC for #6 as there is no reason for more parallel work and tests have been proven to pass.

Crediting quicksketch

It has been a while since I reviewed or tested any of this stuff.

But "small patch first, big patch later" seems like a good idea in general.
It gives us a git history that tells a story. Which is good.

@fabianx I agree that patch #6 is simpler, great starting point, open a new issue for more complex approach by @donquixote .

Patch #6 gives us a big bank for buck, small change, whereas @donquixote approach while more robust and reportedly even better performance is a refinement.

As for the D8 issue
#2720849: D8 improve theme registry build performance

I actually do still believe that there's too much divergence to use the @quicksketch method as it only applies to D7.
See D8 branch 8.1.x
function drupal_find_theme_functions
pay attention to:
// Grep only the functions which are within the prefix group

This change in D8 using $first_prefix is completely new and is a divergence.
Personally, I'd prefer to attack the performance issue as what it is, a performance issue, let the code bases be different, the goal is to have the performance issue resolved in this theme function drupal_find_theme_functions , we should not get hung up on what is inside this function, the goal is to have it work and work well. The @quicksketch patch does just that for D7 , there is a @donquixote patch that works for D8 , D8 and D7 are different and will always be different lets celebrate their differences and make the best of it.

The similarity is, both D7 and D8 have a function called drupal_find_theme_functions , this makes sense

What about we get quicksketchs short patch #73 in for Drupal 8 and then open a new issue again for the more complex approach by donquixote.

For what it is worth, I agree with @joseph.olstad in that my approach is not exactly portable to Drupal 8. Or more correctly, would probably not have as significant (or possibly any) improvement. Because D8 already has getPrefixGroupedUserFunctions() (which D7 does not), it's already running preg_grep() on a limited number of functions instead of the entire function list each time:

Drupal 8:

$matches = preg_grep('/^' . $prefix . '_' . $pattern . '/', $grouped_functions[$first_prefix]);

Drupal 7:

$matches = preg_grep('/^' . $prefix . '_' . $pattern . '/', $functions['user']);

So for the primary benefit of my original patch -- searching on a subset of functions instead of all of them -- D8 already includes this improvement; though written in a different way. This has been in place since #939462: Specific preprocess functions for theme hook suggestions are not invoked was committed, which was about a month before my original D7 patch.

I think a fair approach would be committing my short patch (#6) to D7 immediately, then work on donquixote's approach for both D7 and D8, which might yield better gains.

http://www.azlyrics.com/lyrics/adamsandler/joiningthecult.html

Saving twelve bucks, and a free haircut

I posted a "port" of my patch at #2720849-10: D8 improve theme registry build performance for the purpose of showing what my fix would look like in D8. The main fix is already fixed in D8, so this mostly just optimizes the number of functions returned, which might save some memory but probably isn't much different from a speed standpoint.

@Fabianx , sorry for pushing your buttons so much, but the parent issue says that you actually RTBC'd patch 73 quite a long time back. Now that you're a core maintainer this should be a slam-dunk for you. BTW, your work is excellent quality by the way, appreciate it. Drupal 7.50 while not perfect (whats ever perfect?), it was a very BIG improvement over 7.44, hoping to keep that momentum going. You want a haircut, get yourself a haircut, you're gonna save twelve bucks. Just do it.

***EDIT***

actually you didn't RTBC it, it was mikeytown2

#73: This is great! Can you open another issue?

We unfortunately need to get that into D8 first ...

So we do have another issue for D8, and there is a @quicksketch patch for it based on #73 , although sorry, I haven't tested that yet.
*** EDIT ***

The divergence is by my patch, which was committed first.

I still believe if my patch was rolled back and the postProcessExtension changed, we could use the simpler approach in D8 as well purely.

But I do think we should have the best out of both worlds now in D8. Thanks so much quicksketch!

If the D8 committers decline the patch for whatever reason, I am gonna go ahead and commit #6.

I just want to at least _try_ to get some kind of synchronicity in first ;).

Okay with all? :)

#29: joelpittets benchmarks in the original issue showed a clear benefit for speed, too of your method over mine.

It might depend on the site obviously and in Drupal 7 the number of non-theme related functions is likely way higher than in D8.

Back to RTBC, #6 is pending Drupal 7 commit.

(Will leave in this state for a few days, then commit.)

Also targeting for next bugfix release, so it does not get forgotten accidentally.

+1 .. Nice .. waiting for this.

Re-reviewed with Fabianx -- this is essentially the same patch as http://cgit.drupalcode.org/drupal/commit/?id=80554eb and $prefixes willl always be set.

Committed and pushed to 7.x, thanks!

Woot!