Problem/Motivation

This is a follow-up for issue #1443308.

Drupal suffers from a performance hit, when the code is accessed through an NFS mount due to bugs/issues in PHP:
https://bugs.php.net/bug.php?id=52312
https://echo.co/blog/speed-php-nfs-turborealpath

Proposed resolution

As there is a static cache in module_load_include() now, it is adding files only once. This results in the require_once() call that is being made for inclusion to be obsolete, allowing the use of require() instead, avoiding the issues in PHP.

Note raised by pjcdawkins, in a comment here:
This is valid only in the context of the module_load_include() method only, as files might be included through other means and the static cache to not be populated, resulting in the possibility for double file inclusion.

Remaining tasks

Discussions how it will affect contrib space, RTBC, cmmit.

User interface changes

None.

API changes

Not direct, but developers will have to prefer inclusion of files through module load include instead of plain PHP require_once statements.

Data model changes

None.

CommentFileSizeAuthor
#2 core-issue-2663228-poc-D7-2.patch482 bytesndobromirov

Comments

ndobromirov created an issue. See original summary.

ndobromirov’s picture

ndobromirov commented
Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new core-issue-2663228-poc-D7-2.patch482 bytes

Here is a proof of concept patch.
Updated issue summary.

Status: Needs review » Needs work

The last submitted patch, 2: core-issue-2663228-poc-D7-2.patch, failed testing.

pounard’s picture

pounard
Primary language French
commented

Still think this might break a lot of modules, it's not uncommon to see modules loading a few files in their hook_boot().

pjcdawkins’s picture

pjcdawkins commented

I have always assumed it's valid to run require_once() directly from PHP code when the location is known (i.e. in my own module) - there's no reason to use module_load_include() in that case. So yes I expect that happens in many other modules too.

Double file inclusion is something you'd really want to avoid - it can result in bad side-effects, very hard-to-debug side-effects, or fatal errors.

pjcdawkins’s picture

pjcdawkins commented

And the link you reference https://echo.co/blog/speed-php-nfs-turborealpath looks like a sufficient workaround for the bug you reference. Or is it not?

ndobromirov’s picture

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

Hi,

It was just a brief idea, after I saw the parent issue's code. As I see the breakage it causes and also all the reasonable concerns you are rising, I will be closing this.

Thanks for all the feedback. I will be testing the solution in link 2 in the description, when the case present itself again, as changes to the old project will be hard / impossible to implement.

Some history:
This was in a project that was active 2-3 years ago, it was a complex system that was managed through an AEGIR. The builds from the AEGIR host had to be deployed to a number of web-hosts, so the first idea was to mount the code and during the benchmarks, we experienced the issues above. The solution that is described in one of the links was either not found, not available or not working at that moment, I don't remember... So the solution we've came up with at the time was to rsync the code to the web-heads though some of the AEGIR hooks.

BR.