generate URLs faster

[MegaphoneJon]

This follows on a conversation from January with @jackrabbithanna toward addressing the performance of CiviCRM page loads.

We currently call CRM_Core_Menu::items() each time CRM_Core_System::url() is called. url() is called many times per page - and CRM_Core_Menu::items() is a full rebuild of civicrm_menu - scanning for and loading XML files, Afforms, anything that can add a route.

This patch cuts an average of 10-12% off every page load time - except search results, which were comparable (I'm not sure why). It also doesn't change load time for SearchKit AJAX requests - presumably because URLs are generated in JS, not PHP.

[MegaphoneJon]

Also note this is a heavy site with tons of extensions and modules - I would expect the page load improvement to be more pronounced on a lighter site.

[colemanw]

Ah, our old friend the static array. Good to see you again.

[jackrabbithanna]

Awesome Jon, we're going to test this on a few sites

[demeritcowboy]

It sets this here, but then never uses it again except to check if it's set, and then down below ALWAYS returns $path. Is it missing a return $newPath here? And if so, shouldn't it cache the new path and return that if cached?

[MegaphoneJon]

Oh - hmm, yes. I need to revise this. I did my initial tests against pages that didn't have params (i.e. the route matched civicrm_menu exactly). Then I modified it to support params but didn't go back and revise this.

[MegaphoneJon]

Also...when did this get to be 5 levels of nested ifs? Yeesh.

[MegaphoneJon]

OK - I fixed this and reduced the nested ifs to a max of 3, but I haven't tested the new patch yet. I was actually hoping some CI existed on this repo that would run tests, but oh well. I'll look at this later today.

[demeritcowboy]

5 levels of nested ifs

That's how you know it's civi. No copyright block needed in the file.

hoping some CI existed on this repo

There isn't anything automatic, but what prompted me to take a closer look at the code was that as a sanity check I ran it with civicarrot against the cdntaxreceipts extension's tests. You managed to crash github so that there aren't even logs of the phpunit part, which is no easy task: https://github.com/SemperIT/CiviCARROT/actions/runs/6476564520/job/17585470546. It's running again now: https://github.com/SemperIT/CiviCARROT/actions/runs/6485761313/job/17612539157

civicrm_entity and webform's tests also run in a drupal 8 environment, but I don't think they hit too many civi urls, usually just on the drupal side, so for this PR might not pick anything up.

[demeritcowboy]

It crashed again but not as bad. Normally the run takes about 15 min so I'm guessing it's getting into some loop. Just clicking around manually I don't see anything obvious. I'll try to see what the test issue is about (the config file warning isn't relevant).

[demeritcowboy]

The test issue is that during an initial install of civi, this line generates an infinite loop, because there is nothing in civicrm_menu, and then calling store() ends up back here again recursively while trying to build menu links.

That might also happen during a cache clear - I didn't test that.

[jackrabbithanna]

I did test menu rebuild / rebuild triggers with this patch, and it causes an "out of memory" php error.

[MegaphoneJon]

OK, I pushed a fix for the infinite loop issue.

I doubt the cdntaxreceipts extension calls CRM_Utils_System::url() enough to show a noticeable difference in time taken, nor do I know how even the Github server load is from run to run. But maybe if the stars align we can see some sort of difference in time to run tests?

[demeritcowboy]

Ok it's running now (https://github.com/SemperIT/CiviCARROT/actions/runs/6502732003/job/17662200722) but just noting that I'm not sure the CRM_Core_Menu::get() call lower down will ever return anything now? Since nothing will ever get put into civicrm_menu. I think it would now go like:

1. processInbound
2. self::$routesInitialized = TRUE;
3. Menu::store(), but then before actually storing anything it will call back to...
4. processInbound
5. then it skips over the menu store

[demeritcowboy]

It passed and it's MUCH faster, but I'd want to check out my theory above because if it's true then the whole function could just be replaced with return $path; to be even faster (grin). I'm not too familiar with what this function does - the history suggests it was originally to allow parameters that had slashes in them.

[demeritcowboy]

• It looks like it does do that at the start but then there's a point somewhere else where it does complete the menu population.

  I can't see any downside from it and this is an impressive speed improvement.

• I'm still a little unsure about what this code does. I know that drupal does url params as slash-separated components, and I can see that if I do e.g. civicrm/activity/fee/fi/fo/fum?reset=1&action=add&context=standalone, then it converts it to civicrm/activity/fee:fi:fo:fum, but then I don't know what would be processing that because it's handled by civi not drupal, and this code only acts on civi urls. The url that loads is civicrm/activity/fee/fi/fo/fum?reset=1&action=add&context=standalone, and civi doesn't care about the fee/fi/fo/fum it treats it the same as civicrm/activity. And if this were for extensions it would just use regular url params?

  Are there drupal modules that register or somehow process urls in the /civicrm namespace?

• Lastly there's Coleman's note about static. I don't know if Civi::$statics will work here since the module might be booted before civi is. It's possible static might have some effect on https://test.civicrm.org/job/CiviCRM-D8-Matrix - I'm not sure how process-separated each of those tests are. There is drupal_static, but the way it's implemented leads to confusing code. In drupal 8+ there is the drupal cache api, it just means some situations might need a manual drupal cache clear, but I think by now we're all used to regularly having to clear drupal 8 cache when we change stuff.

  I don't think the static would block this PR, but might be a good followup.

[MegaphoneJon]

It passed and it's MUCH faster, but I'd want to check out my theory above because if it's true then the whole function could just be replaced with return $path; to be even faster (grin). I'm not too familiar with what this function does - the history suggests it was originally to allow parameters that had slashes in them.

I tried returning just /civicrm at some point and it didn't work. I don't know if returning $path would work? I'm way out of my depth Drupal-wise here - it's just I've repeatedly seen this function stand out during profiling as one that takes more time than you'd expect.

[totten]

I don't know if Civi::$statics will work here since the module might be booted before civi is. It's possible static might have some effect on...

Right. Civi::$statics is generally better with tests (that enable/disable extensions/modules) but it's usu unavailable pre-boot. OTOH, for composer-style site-builds, the autoloader is already setup. So a straight swap (self::$knownPaths ==> Civi::$statics[__CLASS__]['knownPaths']) just might work.

But to be extra cautious, one could say:

-      if (isset(self::$knownPaths[$path])) {
-        return self::$knownPaths[$path];
-      }
+      if (class_exists('Civi', FALSE) && isset(Civi::$statics[__CLASS__]['knownPaths'][$path])) {
+        return Civi::$statics[__CLASS__]['knownPaths'][$path];
+      }

Even if Civi::$statics isn't available at the start of the first call to processInbound(), it would be available very shortly (as soon you call $civicrm->initialize()), and it will be available on subsequent calls to processInbound().

[demeritcowboy]

Ah that seems like a quick win on the static, so with that updated I'd be good to give a merge-ready pending some sanity runs and any thoughts from @jackrabbithanna.

[jackrabbithanna]

Tested again, and now no problems on cache clears and menu / trigger rebuilds.
It does seem much faster
/civicrm/some/path/fee:fi:fo:fum behavior is the same now as before this patch, so not a problem IMO

We've put this patch on a site we are actively developing on to put it through its paces

[MegaphoneJon]

We've put this on a couple of sites, and found an issue. Not every path is in civicrm_menu. There are a couple of paths hard-coded into CRM_Core_Menu::get(). So this patch fails when importing contacts because civicrm/queue/runner can't be found by this patch. Presumably /report/instance also has issues.

Reading through the code, I'm not sure why this matters, since we're calling CRM_Core_Menu::get() but I guess now we know what to write tests for.

[MegaphoneJon]

I decided to see if I could get this function to just return $path and it has the same issue as this PR. But I found the reason - civicrm.routing.yml specifies \Drupal\civicrm\Routing\Routes::listRoutes as a callback. Which also uses CRM_Core_Menu::items(), and is missing the hard-coded paths available in CRM_Core_Menu::get().

If the route is already in the routing table, we can just return $path from the InboundProcessor. and the normal routing code kicks in and finds the path in routing.

I'm trying to see if we can a) remove all the routes but civicrm or b) calling CRM_Core_Menu::get() in some hypothetical route-altering listener I'm going to look for.