As per Rasmus' comments in Szeged, require()/include() are slightly faster than require_once()/include_once(), as they do not require a lookup table.

The registry should never trigger including a file that is already loaded, as the code in it would already be available and the registry wouldn't try to include the file a second time. Therefore we don't need to use require_once(), and can just use require(). The attached patch does so.

This should be a minor performance improvement. There's probably lots of other places we can drop the _once(), but one at a time. :-)

CommentFileSizeAuthor
#4 mw.patch915 bytesmoshe weitzman
require_registry.patch1.23 KBCrell

Comments

Anonymous’s picture

Anonymous (not verified) commented

bookmark

moshe weitzman’s picture

moshe weitzman commented
Status: Needs review » Needs work

I am getting WSOD on the modules page with this patch, The error in the logs is [apc-error] Cannot redeclare class queryconditioninterface. I traced to openConnection() in includes/database/database.inc. Line 851 says require_once DRUPAL_ROOT . '/includes/database/' . $driver . '/database.inc';. Then the registry does a require on that same file and fatal WSOD ensues. Not sure how to fix this, since registry is obviously not available before the DB is bootstrapped. Make an exception for the db_driver file?

Crell’s picture

Crell commented

A better question, IMO, is why the registry is doing a require on that file if QueryConditionInterface is already loaded. The registry shouldn't fire unless it's not loaded, shouldn't it?

moshe weitzman’s picture

moshe weitzman commented
Status: Needs work » Needs review
StatusFileSize
new mw.patch915 bytes

Yes, that is a better question.

I think the culprit is registry_load_path_files() which requires all files that were cached for the current path. It needs to check if those files were already loaded. Or we drop that hunk from your patch as I have done in the attached patch. Arguably that defeats much of the purpose of your patch but the only better way I can see is to special case the DB files.

mfer’s picture

mfer commented

subscribe.

Crell’s picture

Crell commented

Hm. OK, I see what the issue is there. Well yuck.

That's going to break for anything that is lazy loaded before the menu handler fires. At the moment that's probably just database.inc, but hypothetically could be anything else, especially as we add more classes.

The simple solution, and what we should probably go ahead and do first, is Moshe's patch in #4.

The longer-term solution is probably to move the autoloading earlier, before menu_execute_active_handler() but still using the current menu item. Hrm. I don't know how early we'd move it, though, and it could conflict with the defined database.inc include in bootstrap.

Anonymous’s picture

Anonymous (not verified) commented

The other solution would be to wrap the included classes in

if (!class_exists('myClass')) {
  class myClass {
    ...
  }
}

but is it worth the trouble?

mfer’s picture

mfer commented

@earnie I think this approach introduces more complexity than we need or would be good. Plus, it's a pain for DX.

Here's a thought... since the database files are manually included (so we can even get to the registry of files to include) what if those files were excluded from the registry builder.

dave reid’s picture

dave reid
he/him
Primary language English
Location Nebraska USA
commented

After taking a look through a lot of articles, it seems like there is less and less of an anti-require_once movement now that PHP has improved the performance of it in 5.2 (and will supposedly be even better in 5.3). We should take a look into simply adjusting the realpath-cache-size and realpath-cache-ttl PHP directives to potentially improve performance without taking a hit in the DX.

http://www.techyouruniverse.com/software/php-performance-tip-require-ver...
http://arin.me/php/php-require-vs-include-vs-require_once-vs-include_onc...
http://www.phpguru.org/article.php/219
http://www.nabble.com/require_once-performance-issue-in-ZF-td19218102.html

Crell’s picture

Crell commented

class_exists() also triggers autloading and therefore the registry, unless you pass FALSE as the second parameter. That won't help here. It's also why there was a huge performance hit in the original patch until we removed that. :-)

Anonymous’s picture

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

The last submitted patch failed testing.

Anonymous’s picture

Anonymous (not verified) commented

the landscape for this patch has changed now that #325665: Cache registry lookups has landed. the registry is now a true lazy-load system.

casey’s picture

casey commented
Status: Needs work » Closed (won't fix)

There's no drupal_function_exists() any more since we dropped regisrty for functions.

Fixed as we dont use require_once any more :p

vesapalmu’s picture

vesapalmu commented

I don't want to reopen this issue, but if you just ended up here by googling the error text (I just did):
"PHP Fatal error: require() [function.require]: Cannot redeclare class queryconditioninterface in /var/aegir/drupal-7.0/includes/database/database.inc on line 1648".

It is likely that your fix with Drupal 7.0 is disabling apc.include_once_override. Just remove or comment out line:
apc.include_once_override = 1
from /etc/php/conf.d/apc.ini or where ever you keep your apc config.

joshk’s picture

joshk commented

Thanks @wesku

zzadik’s picture

zzadik commented

Thanks wesku.
Had the same issue, and your solution worked a charm