[6.x] Remove wasted file read when loading package manifest #32646
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Filesystem get was introduced in laravel#25012 as a locking mechanism to prevent partial reads of the manifest file. In laravel#26010 the locking was substituted by an atomic rename-based file write. The get called survived the refactor, resulting a noop file_get_contents that just trashes the output. This commit removes the read entirely.
TheNodi
force-pushed
the
manifest-remove-file-read-2
branch
from
406b2d5
to
736ab17
Compare
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
See #32645
This PR removes an unused
file_get_contentsof the packages manifest file.How / Why
I was optimizing an application to have zero disk accesses, but I noticed the package manifest was read on every request, even though it is a PHP file and it should be cached by opcache.
I dug into the code and I find out there's a get of the manifest file before requiring it:
framework/src/Illuminate/Foundation/PackageManifest.php
Line 109 in 407372d
This translates to always reading the file into memory, and then throwing away the result.
$this->files->getwas introduced in commit 683637a as a locking mechanism to prevent partial reads of the manifest file.In commit 633a907 the locking was substituted by an atomic rename-based file write. The get called survived the refactor, resulting a no-op
file_get_contentsthat just trashes the output.Behavior Differences
When the build function is not capable of creating the file, the filesystem get would have thrown a
FileNotFoundException. After this changes, an empty manifest is returned instead.I don't think there is a way for build to silently fail a write, and the following line is explicitly checking the existence of the file. Hence I believe this is the intended behavior.