Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[ProxyManager] performance optimization for lazy loaded services #9596

Closed
wants to merge 1 commit into from

Conversation

lsmith77
Copy link
Contributor

Q A
Bug fix? no
New feature? yes (well more a performance enhancement
BC breaks? no
Deprecations? -
Tests pass? -
Fixed tickets -
License MIT
Doc PR -

Result of a discussion at DrupalCamp Vienna.
The idea is essentially to no longer have to use a proxy once the service has been loaded.

Given a lazy loaded service foo with a method foo():

// proxy
var_dump(get_class($this->container->get('foo')));
// still proxy
var_dump(get_class($this->container->get('foo')));
$this->container->get('foo')->foo();
// no longer a proxy
var_dump(get_class($this->container->get('foo')));

Haven't looked at the tests yet, wanted to get feedback if this makes sense first.

/cc @dawehner @Ocramius

@stof
Copy link
Member

stof commented Nov 24, 2013

I see an issue here:

$foo = $c->get('foo');
$foo->init();

$foo2 = $c->get('foo');

// No, $foo !== $foo2

@lsmith77
Copy link
Contributor Author

Right .. but how relevant is this?

Sem related, do we have a helper class similar to the doctrine utility class to find the "actual" class name of a proxy? Seems to be like this kind of situation is just an expected side effect of using proxies.

@Ocramius
Copy link
Contributor

I explicitly coded proxies this way to have $c->get('foo') === $c->get('foo') regardless of what happens in between. This may lead to very nasty bugs and wtfs. I'm 👎 to this approach

@lsmith77
Copy link
Contributor Author

when would this ever be relevant? seems like a very theoretical issue. then again the benefits of this are also fairly rare, could be an option, i.e. lazy: optimized (optimized is probably not a good way to name this).

@Ocramius
Copy link
Contributor

@lsmith77 as for the proxy class resolution, there's ClassNameInflector. Why are you trying to resolve the real name though?

@Ocramius
Copy link
Contributor

@lsmith77 it's not that it's relevant, it's consistent. Are you trying to remove the overhead of proxy calls for a specific method? If so, why are you doing that? Is it really that much of an overhead?

@lsmith77
Copy link
Contributor Author

i was just making the point that a proxy already introduces some changes to the code that gets the dependency injected which made me wonder if we have a solution to get the original class name.

@lsmith77
Copy link
Contributor Author

yes .. the idea is to reduce the method calling overhead when using the same proxied service in several places.

@mnapoli
Copy link
Contributor

mnapoli commented Nov 24, 2013

IMO this is not a problem ($foo1 != $foo2). The service container provides you a service for a given name. You shouldn't make assumptions on what the container gives you.

It's just like when programming against an interface. You ask for a XXXInterface, and you shouldn't care what implementation you get. If you do, then the whole point is moot.

If you start having expectations on what the container gives you, then you are not doing Inversion of Control.

@Ocramius
Copy link
Contributor

@lsmith77 I understand the problem you are trying to solve, but I'm wondering if it's worth solving it. I'd consider this way only if it's really a concern (once there is something like an xdebug trace to back it, for example).

Consider that a virtual proxy as of proxy-manager has a typical method override like following:

public function doFoo($bar)
{
    $this->initialized || $this->initialize(...);

    return $this->wrapped->doFoo($bar);
}

The overhead here is 1 empty method call and one boolean operation that resolves immediately after the first initialization.

I wouldn't try to introduce (possible) new problems (such as breaking identity of a service) to fight such a minimal overhead.

I'm already keeping an eye on those: https://travis-ci.org/Ocramius/ProxyManager/jobs/14099036#L222

@Ocramius
Copy link
Contributor

@mnapoli I agree with that, but when using the container as a service locator you are doing some assumptions of this type, and the container works indeed as a service locator as well.

Religious beliefs about IOC are not helping IMO

@@ -76,6 +76,7 @@ public function getProxyFactoryCode(Definition $definition, $id)
$instantiation new $proxyClass(
function (&\$wrappedInstance, \ProxyManager\Proxy\LazyLoadingInterface \$proxy) use (\$container) {
\$wrappedInstance = \$container->$methodName(false);
\$container->set('$id', \$wrappedInstance);
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

$id should be var_exported by the way

@fabpot
Copy link
Member

fabpot commented Dec 29, 2013

What about this one? Do we all agree that it's not worth it?

@Ocramius
Copy link
Contributor

@fabpot it's not needed in my opinion. Services being heavily used shouldn't be proxied anyway.

@fabpot fabpot closed this Dec 29, 2013
@donquixote
Copy link
Contributor

Interesting discussion.
(I hope this issue being closed does not mean we can't continue the conversation for a bit)
Reminds me of Ocramius/ProxyManager#35 (attention: long read)

+                    \$container->set('$id', \$wrappedInstance);

I think this won't be sufficient.
If a number of services already have a reference to the proxy, they won't be updated.
I think the logic should happen in the consumer service, not on container level. E.g.

class MyService {
  function __construct(Thingie $thingie) {
    $this->thingie = $thingie;
    if ($thingie instanceof ProxyThingie) {
      $thingie->proxyObserveInstantiation(function($thingieInstance) {
        $this->thingie = $thingieInstance;
      });
    }
  }
}

it's not needed in my opinion. Services being heavily used shouldn't be proxied anyway.

I think you are right most of the time.
An example would be a service that will only be instantiated on a cache miss, has a measurable instantiation cost, and will be used heavily once instantiated.
Let's say on 33% of projects where this is used, the cache is active, so it almost never gets instantiated.
On another 33% of projects the cache is not active (because of some constraints), so it always gets instantiated.
And on the remaining projects, it is instantiated on every second request or so.. whatever.

The point is, as a component developer you cannot predict how regularly the service will be instantiated.
(And as an end product developer, you don't really know because you are just using the CMS and have no idea)

This doesn't mean I want to see this in Symfony. Just giving some food for thought.

@Ocramius
Copy link
Contributor

@donquixote my point is that the developer should consider whether the performance hit is instantiation or amount of calls.

@donquixote
Copy link
Contributor

Yes. But maybe the developer does not know yet. E.g. if this is a CMS, then only the end project maintainer will know, and this might be a "Joe Blogger" type of person.

@Ocramius
Copy link
Contributor

@donquixote doesn't the SF2 configuration allow overrides on service definitions? The flags can be customized at application level IIRC

@donquixote
Copy link
Contributor

The "application level" on a CMS scenario (e.g. Drupal) is not the same as on a framework scenario.
There is the CMS developer who does not know what project specifics to optimize for, and there is the site maintainer who does not know what a proxy is.

I am just saying there are cases where the dual case optimization can be useful. Maybe it is good enough to do this as a one-off implementation in the respective CMS, so there is nothing to do here.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants