Prevent loading class_exists multiple times #161
Conversation
|
As already stated on that thread, the correct solution would be to add a new specialised endpoint. |
| // Reset our static cache now that we have a new loader to work with | ||
| self::$failedToAutoload = []; | ||
| self::$loaders[] = $callable; | ||
| if ($callable !== 'class_exists' || !self::isLastLoaderClassExists()) { |
There was a problem hiding this comment.
Please invert this condition and make it early-return. Also space around ! is required.
| return false; | ||
| } | ||
| $lastLoader = self::$loaders[$count - 1]; | ||
| if (!is_string($lastLoader)) { |
There was a problem hiding this comment.
Please remove this condition, it's not an optimization and is redundant.
| protected static function isLastLoaderClassExists() : bool | ||
| { | ||
| $count = count(self::$loaders); | ||
| if (0 === $count) { |
There was a problem hiding this comment.
Here no yoda used will be.
In order to avoid a BC break this only checks for duplicate class exists loaders at the bottom of the stack.
jrjohnson
force-pushed
the
145-class-exists
branch
from
5de8458
to
33a492a
Compare
|
Thanks @Majkl578 I made those changes. @Ocramius thanks for the feedback. My thinking is that adding a new |
Yes, that is something that was already clear to me - I realize that I'm taking the non-DX solution, but the repeated registration of loaders is something to be fixed elsewhere anyway (like keeping a Overall, less magic workarounds, especially in a general-purpose library that is basically used everywhere. |
|
This isn't about developer experience it is about where the bug is. The performance killing bug is in this library. It should be fixed here. More importantly it can be fixed here for a huge number of situations with no risk. |
|
From my PoV, the bug is calling the register loader multiple times. The
thing is doing exactly what it should do.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
…On Mon, Dec 4, 2017 at 6:31 PM, Jonathan Johnson ***@***.***> wrote:
This isn't about developer experience it is about where the bug is. The
performance killing bug is in this library. It should be fixed here. More
importantly it *can* be fixed here for a huge number of situations with
no risk.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#161 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakDuZkoh4MG7OH009YPBEX4LYLFLGks5s9CxZgaJpZM4Q0EQY>
.
|
|
@Ocramius the bug to me is the fact that this library requires using a global state to register loaders. Having to add a global state in Symfony itself to workaround a performance issue related to the global state of this library looks weird to me. |
|
That is indeed something that can be fixed in 3.x by simply not requiring a
loader at all, but for now a loader API that specifically de-duplicates its
internal spl autoloader stack is indeed the fix.
Whether symfony leverages it or not later on is a completely different
question.
Marco Pivetta
http://twitter.com/Ocramius
http://ocramius.github.com/
…On Mon, Dec 4, 2017 at 6:41 PM, Christophe Coevoet ***@***.*** > wrote:
@Ocramius <https://github.com/ocramius> the bug to me is the fact that
this library requires using a global state to register loaders. Having to
add a global state in Symfony itself to workaround a performance issue
related to the global state of this library looks weird to me.
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#161 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakNDdSBU7NFASoLXa9970SYGeONtWks5s9C7NgaJpZM4Q0EQY>
.
|
|
Well, why forcing to migrate to a new API (that will also disappear in 3.x) when you can make all projects out there benefiting from a better performance by just upgrading, in case they follow the recommendation of registering |
|
Because the current API is behaving as expected?
…On 4 Dec 2017 18:53, "Christophe Coevoet" ***@***.***> wrote:
Well, why forcing to migrate to a new API (that will also disappear in
3.x) when you can make all projects out there benefiting from a better
performance by just upgrading, in case they follow the recommendation of
registering class_exists itself as loader (i.e. doing what Doctrine 3
will always do)
—
You are receiving this because you were mentioned.
Reply to this email directly, view it on GitHub
<#161 (comment)>,
or mute the thread
<https://github.com/notifications/unsubscribe-auth/AAJakK_lgFUU9LwqNEuSODQDyQgpX75Iks5s9DGlgaJpZM4Q0EQY>
.
|
|
Handled in #162 |
Implementing @stof's suggestion in #145 (comment)
This is just a pre-2.0 bandaid to help solve the biggest issue where
class_existsis added as a loader automatically many many times which is a huge performance killer in testing symfony apps (symfony/symfony#24596). While #137 solves this in applications it doesn't help with tests since they maintain the global static state and start up multiple kernels (and registered loaders) at the same time.In order to avoid a BC break this only checks for duplicate class exists
loaders at the bottom of the stack.