Background here and lots of other places, this was also something Rasmus picked up in his Drupalcon keynote.

Not sure why we do require_once in _registry_check_code() when it already maintains a record of which files it has loaded during the request, if it's to protect against badly behaved modules doing their own include/require then I think we should put the responsibility for that behaviour onto them.

It looks like webgrind has require and include coming in a bit faster (tested with APC enabled), but I'm not up on using strace which is probably necessary to really verify this.

CommentFileSizeAuthor
#13 require_once-4.patch2.57 KBc960657
#11 require_once-3.patch2.58 KBc960657
#9 require_once-1.patch2.97 KBc960657
head.png382.17 KBcatch
patch.png388.1 KBcatch
patch.png430.28 KBcatch
require.patch920 bytescatch
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Status: Active » Needs review
catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented
Issue tags: +Performance
Crell’s picture

Crell CreditAttribution: Crell commented

The reason for the _once is that very early in the bootstrap we were running into duplicate load requests on, I think, the DB layer.

I believe the issue was that with the front-loading we had at the time, we'd end up with the following logic:

1) Page makes use of UpdateQuery or some other conditional part of DBTNG.
2) Registry caches the need for query.inc.
3) New page is requested.
4) Very early bootstrap triggers an update query. query.inc gets lazy-loaded.
5) menu_execute_active_handler() front-loads all of the cached files.
6) One of those files is query.inc, which was already loaded in step 4.
7) Drupal go boom.

Perhaps we could bring back the front-cache and push it much earlier in the bootstrap than menu_execute_active_handler() (it could probably use its own bootstrap phase right after the DB initializes), but otherwise I suspect the two will bump into each other.

Actually that's probably a good idea anyway. If we add a DRUPAL_BOOTSTRAP_REGISTRY phase right after DRUPAL_BOOTSTRAP_DATABASE, we can move the spl_autoload_register() calls to there and follow that immediately with loading the front-cache, if we manage to bring that back. That should avoid any collisions.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

Shouldn't all files be loaded via _registry_check_code() anyway? Or does spl_autoload_register() do that itself? If not, we probably need a central routing function for loading files anyway, like drupal_load() for example.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented

catch, i'll try to get an strace run going in the next couple of days.

edit - took the front-cache bit and put it in the right issue: #497118: Remove the registry (for functions)

Crell’s picture

Crell CreditAttribution: Crell commented

spl_autoload_register() is what wires up the class/interface autoload functions. I don't recall the exact details beyond what I listed above, as the code has changed a non-small bit since then.

justin: Not off hand. When the registry first loaded it did have front-loading. It was taken out later. So you could go back to the original patch and see how that worked.

Oh yeah, and I found the issue I mentioned previously in the other thread regarding cache vs. non-cache include files: #329026: Differentiate between cacheable and non-cacheable registry files

That's right around when the front caching was removed anyway, making that issue redundant.

catch’s picture

catch
he/him
Primary language English
CreditAttribution: catch commented

While stracing, I'd be interested in more data on http://nirlevy.blogspot.com/2009/01/slow-lstat-slow-php-slow-drupal.html - seems like we'll really get hit by that if we break things into more includes, so might be worth an ini_set somewhere.

Crell’s picture

Crell CreditAttribution: Crell commented

Unfortunately, the realpath_cache_size is per-system, so we can't override it in Drupal or htaccess: http://us2.php.net/manual/en/ini.list.php

Quite sad, since that sounds like a potentially huge win on the performance front. :-(

c960657’s picture

c960657 CreditAttribution: c960657 commented
FileSize
require_once-1.patch2.97 KB

How about checking for the existence of the requested class/interface early in _registry_check_code()?

Status: Needs review » Needs work

The last submitted patch failed testing.

c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
require_once-3.patch2.58 KB

Reroll.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Needs work

looks good to go to me, nitpicks only:

+  if ($type == 'class' && class_exists($name) ||
+      $type == 'interface' && interface_exists($name)) {
+    return TRUE;
+  }

should be like this to match the rest of core:

  if (($type == 'class' && class_exists($name)) || ($type == 'interface' && interface_exists($name))) {
    return TRUE;
  }
c960657’s picture

c960657 CreditAttribution: c960657 commented
Status: Needs work » Needs review
FileSize
require_once-4.patch2.57 KB

The if clause is now on one line.

Anonymous’s picture

Anonymous (not verified) CreditAttribution: Anonymous commented
Status: Needs review » Reviewed & tested by the community

i prefer the extra brackets for readability (yes, && has higher precedence than ||, so this is an optimisation for readability only), and grep seems to say core does as well, but i don't think that should hold up this patch.

lets see whether webchick or dries disagrees, RTBC.

Dries’s picture

Dries CreditAttribution: Dries commented
Status: Reviewed & tested by the community » Fixed

Committed to CVS HEAD. Thanks.

Status: Fixed » Closed (fixed)
Issue tags: -Performance, -Registry

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

Issue tags: +Performance, +Registry

Restoring issue tags, see #2125755: System messages removed all issue tags during D7 upgrade.

donquixote’s picture

donquixote CreditAttribution: donquixote commented
Issue summary: View changes 
  if ($type == 'class' && class_exists($name) || $type == 'interface' && interface_exists($name)) {
    return TRUE;
  }
How about checking for the existence of the requested class/interface early in _registry_check_code()?

Does anyone remember why, and whether we still need this?