backdrop-issues icon indicating copy to clipboard operation
backdrop-issues copied to clipboard

[PS] Optimize settings.php discovery

Open klonos opened this issue 2 years ago • 7 comments

I just bumped into this old issue again: https://www.drupal.org/project/drupal/issues/1055856

It was initially filed against D7, and there was a patch for it, with some benchmark results as well:

...running benchmarks several times I see the patched version consistently 5-10% faster except for a couple of outliers. xhprof suggests about the same saving (some from file_exists(), some from the surrounding logic).

The issue eventually got moved to the 8.x-dev queue and lost traction since.

Might be worth porting the D7 patch and doing some benchmarks to see if there's actually any performance gain 😉

klonos avatar May 30 '23 20:05 klonos

Well, the PR didn't break any tests, and nor did it break the installation process (tested both scripted via drush/bee, as well as manually via the web installer): https://github.com/backdrop/backdrop/pull/4447

Now we need to benchmark before/after and see if there are any performance improvements.

klonos avatar May 30 '23 21:05 klonos

@hosef would you be willing to do some benchmarks on this PR? :)

jenlampton avatar Jun 01 '23 22:06 jenlampton

Note to self: https://github.com/backdrop-contrib/webprofiler

klonos avatar Jun 02 '23 12:06 klonos

@klonos could you please rebase your PR against 1.x.

hosef avatar Jul 08 '24 14:07 hosef

@hosef i was able to push the "update branch" button to get this rebased

laryn avatar Jul 08 '24 15:07 laryn

Ok, I did some testing and while this might make finding the settings.php file faster, I think that most of the time there will be no functional difference.

The patch is only 5% faster in a very specific use case where the page just loads the settings file and then exits. Here is the index.php I used to test it.

define('BACKDROP_ROOT', getcwd());

require_once BACKDROP_ROOT . '/core/includes/bootstrap.inc';

backdrop_bootstrap(BACKDROP_BOOTSTRAP_CONFIGURATION);
exit();

This would likely be more impactful for large multisites like the one used by University of California, but in my case where I only have ~50 sites it barely registers at all.

hosef avatar Jul 08 '24 17:07 hosef

Thanks for the rebase @laryn and for the testing/profiling @hosef 🙏🏼 ...wondering if there's a way to profile this by creating 100s or even 1000s of sites in order to mimic a multisite situation 🤔

In any case, I just pushed a commit to fix a PHPCS nag, and I'll leave the PR there for now. Setting the milestone, so that we can decide if we want this micro-improvement or not.

klonos avatar Jul 09 '24 01:07 klonos