service proxies / lazy services #2995

Closed
wants to merge 15 commits into
from

Conversation

Projects
None yet
7 participants
Member

Ocramius commented Nov 17, 2012

Scope

This PR introduces a huge performance tweak for the ServiceManager.

It is a service proxy generation logic based on top of the current proxy generation I am using for doctrine/common#168 (not yet merged, and this cannot be merged until doctrine/common 2.4 is out).

Basically, each service configured to be lazy is wrapped in a service proxy before being returned. This delays instantiation until the service is actually used, which is what most users currently achieve by directly accessing the service manager from within their own services.

My primary aim is to reduce usage of service location (which I personally started considering harmful) in favour of dependency injection.

Example

You can see a complete example of how this works in the results of the PHPPeru hackday I had with @cordoval in this blogpost.

Here's a recap:

  • service location for performance (bad approach):

     class MyService implements ServiceLocatorAwareInterface {
         /* ServiceLocatorAwareInterface stuff */
         public function veryHeavyMethod() {
             // Houston, we got a problem!
             $this->getServiceLocator()->get('something')->doFoo();
         }
     }
  • hard dependencies (good approach, but slow) (this PR fixes the implied slowness)

     class MyService {
         public function __construct(Something $something) {/*...*/}
         public function veryHeavyMethod() {
             $this->something->doFoo();
         }
     }

Example of proxy

I included an example of a generated proxy class at https://gist.github.com/4098721

Testing

I've tested it and it works quite fine. Please let me know your thoughts, since this is quite a big shot :)

Any access to a method or property of the proxy goes directly to the service.

Serializing/unserializing proxies works/tested.

Cloning proxies works/tested.

Todos

  1. un-serialization of proxy services (PHP_Incomplete_Class) (not sure if needed)
  2. proxy factory registered by default in the service manager?
library/Zend/ServiceManager/ServiceManager.php
@@ -591,7 +619,7 @@ public function canCreateFromAbstractFactory($cName, $rName)
foreach ($this->abstractFactories as $index => $abstractFactory) {
// Support string abstract factory class names
if (is_string($abstractFactory) && class_exists($abstractFactory, true)) {
- $this->abstractFactory[$index] = $abstractFactory = new $abstractFactory();
+ $this->abstractFactories[$index] = $abstractFactory = new $abstractFactory();
@Maks3w

Maks3w Nov 17, 2012

Member

Can you submit this fix in a different PR for merge it early?

@Ocramius

Ocramius Nov 17, 2012

Member

Sure thing :) Will try to test this one too

Member

Maks3w commented Nov 17, 2012

and I guess that is needed some option for avoid proxy regenerating in production and a command for force the regeneration.

Sincerely I'm not feel comfortable with Doctrine magic proxies

Member

Ocramius commented Nov 17, 2012

@Maks3w proxies would only be generated once anyway (first access to a proxy object). The only way to pre-generate them would be to iterate over all existing lazy services at least once, as I do in OcraDiCompiler (I can tell you it fails often :( Damn initializers).

I've been working over a month on this proxy implementation (the proxy implementation itself, not this PR) and tested every corner of it.
If you can come up with weird use cases that I cannot solve go ahead and tell me, it only helps me ;)

Otherwise, yes, having doctrine/common being a dependency of the ServiceManager may be a big bummer.

It would be awesome to have this in the 2.1.* series, since it solves a problem that people currently solve the wrong way (IMO).

Member

Ocramius commented Nov 17, 2012

Included 6f9a66e to trigger a successful build - will strip the commit before merging

Contributor

bakura10 commented Nov 17, 2012

As I told you on IRC, I'm 100% for this feature. It would be the best of both worlds :

Good for performance but leads to a lot of boilerpalte and repeated code (this is what I'm doing currently and in some services I have a lot of dependencies, this is really ugly and error-prone).

class MyService
{
    protected $mapper;

    public function getMapper()
    {
        if ($this->mapper === null)
            $this->mapper = $this->sm->get('Mapper');

        return $this->mapper;
    }
}

And dependencies through controller that obvisouly create objects that may not be needed.

I hope everyone will agree on this one =).

Owner

weierophinney commented Nov 18, 2012

I'm going to be honest: I don't get it.

Based on the writeup and the gist, I'm unclear exactly what this is supposed to solve. Additionally, in looking at the code, I'm having a lot of difficulty seeing how this would create a performance boost. From your own writeup, you indicate you want to encourage developers to use DI vs. service location -- and that's been the same goal I've been messaging in my talks as well as in documentation (the notable exception is the tutorial -- and I think the tutorial should be refactored to remove service locator usage).

When developers ask me what to do about cases where a dependency is conditional based on the action selected, I encourage doing one of two things already:

  • break the controller into multiple controllers. If there are different dependencies based on action, then the actions are not all that related, and should be in separate classes.
  • create separate factories per action, with different service names. Since routes specify a controller service name, this is trivially easy to accomplish, and easy for developers to pick up and implement.

These approaches are two ways to squeeze in some extra performance quite easily, and require no extra coding in the framework itself.

From what I can tell, your proposal is trying to address the above use cases. If so, my worries are:

  • a level of indirection both when coding and debugging. Developers need to know proxies are in place, what a proxy is in the first place, and then worry about step-tracing through the call stack. It's a lot more complex.
  • you're introducing a hard dependency in core on a third-party library. This means you cannot just download a zipball/tarball of ZF2 and get it running -- you must also download Doctrine\Common. Considering how many complaints I've heard about Composer from self-proclaimed newbies, and the fact that it complicates the install process for them, I can't imagine what they'll say about this.

The idea definitely looks interesting. However, I'd like to better understand what your goals are, and why they cannot be accomplished in other ways, before we consider merging this. As you note, it's a big change. If we're going to make such a change, I want to be sure we consider other approaches (if there are any, or if any other approaches are indeed needed), and what the impact will be for developers with less than advanced skillsets.

Contributor

bakura10 commented Nov 18, 2012

Just a note for the dependency : we had a talk with @Ocramius about that, and I think the idea is just to make this a feature for developers that would like to use it, exactly like form annotations.

I let ocramius for the rest =).

Owner

weierophinney commented Nov 18, 2012

@bakura10 If that's the case, we then need to also add checks for doctrine/common in the code, raising exceptions when somebody tries to register a lazy service without that library being available. /cc @Ocramius

Member

Ocramius commented Nov 18, 2012

On 18 November 2012 23:42, weierophinney notifications@github.com wrote:

I'm going to be honest: I don't get it.

Based on the writeup and the gist, I'm unclear exactly what this is
supposed to solve. Additionally, in looking at the code, I'm having a lot
of difficulty seeing how this would create a performance boost. From your
own writeup, you indicate you want to encourage developers to use DI vs.
service location -- and that's been the same goal I've been messaging in my
talks as well as in documentation (the notable exception is the tutorial --
and I think the tutorial should be refactored to remove service locator
usage).

When developers ask me what to do about cases where a dependency is
conditional based on the action selected, I encourage doing one of two
things already:

  • break the controller into multiple controllers. If there are
    different dependencies based on action, then the actions are not all that
    related, and should be in separate classes.

Correct, but most devs still won't do it. It's easy to get addicted to
accessing the service locator (see ZfcUser and its giant blob of calls to
the service locator)

  • create separate factories per action, with different service names.
    Since routes specify a controller service name, this is trivially
    easy to accomplish, and easy for developers to pick up and implement.

Correct also here, but it becomes problematic to track down huge
dependency graphs. Putting a "stop" (proxy) somewhere actually helps
breaking the instantiation madness that may happen.

These approaches are two ways to squeeze in some extra performance quite
easily, and require no extra coding in the framework itself.

From what I can tell, your proposal is trying to address the above use
cases. If so, my worries are:

  • a level of indirection both when coding and debugging. Developers
    need to know proxies are in place, what a proxy is in the first place, and
    then worry about step-tracing through the call stack. It's a lot more
    complex.

Agreed, it's a call more in your stack, and the first time you see it,
you're gonna "wtf" at it. It doesn't do anything fancy though.

  • you're introducing a hard dependency in core on a third-party
    library. This means you cannot just download a zipball/tarball of ZF2 and
    get it running -- you must also download Doctrine\Common.
    Considering how many complaints I've heard about Composer from
    self-proclaimed newbies, and the fact that it complicates the install
    process for them, I can't imagine what they'll say about this.

Agreed also on that. I may move this outside ZF2 by putting it in
DoctrineModule if that's preferred, but the patched lines in the
ServiceManager are required to get to that... Doctrine\Common is just an
implementation of the proxy pattern, any abstract factory with a particular
name can do.

The idea definitely looks interesting. However, I'd like to better
understand what your goals are, and why they cannot be accomplished in
other ways, before we consider merging this. As you note, it's a big
change. If we're going to make such a change, I want to be sure we consider
other approaches (if there are any, or if any other approaches are indeed
needed), and what the impact will be for developers with less than advanced
skillsets.

I will come up with a blogpost (hopefully tomorrow) that explains this
patch and generally what I've worked on the last month in depth. In the
mean time, here's a fancy thing for you: this actually allows cyclic
dependencies :D


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/2995#issuecomment-10493855.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Member

Ocramius commented Nov 18, 2012

The optional dependency is probably not required at all if I move this code out of ZF2 and into DoctrineModule.

The only requirement here would be https://github.com/Ocramius/zf2/blob/8b58ed45fa09c413558c1ad88db8c3ecefaef5bc/library/Zend/ServiceManager/ServiceManager.php#L492-L499, which basically means making it possible to define a factory that handles service proxy (or generic lazy services) instantiation.

That done, the rest can be done by simply requiring a module providing that factory.

Owner

weierophinney commented Nov 19, 2012

@Ocramius Can you please clarify the "performance" claim you make, as well as provide a concrete example showing:

  • how I typehint in the consuming object
  • how I create my service in the service manager
  • any steps I need to take to generate proxies (if any)

I also wonder if you can demonstrate the performance impact of generating proxies over simply injecting the required object -- particularly against using the approaches I outlined.

(Maybe I'm being thick, but I'm simply not grasping the concept; if I cannot, how can we expect consumers of the framework to understand and use it?)

Member

Ocramius commented Nov 19, 2012

@Ocramius https://github.com/Ocramius Can you please clarify the
"performance" claim you make, as well as provide a concrete example showing:

  • how I typehint in the consuming object

No changes in type hints are needed. Since the proxy extends the original
service, it is a valid replacement for the original one, and your code is
unaware of being dealing with a proxies (even when dealing with public
properties, that is also handled).

  • how I create my service in the service manager

You request the normal service by its name. The proxy factory interferes in
this process by replacing the actual instance with a proxy. You didn't even
know it happened.

  • any steps I need to take to generate proxies (if any)

That's handled by the proxy factory itself. The first time you request a
proxy, the real service gets generated (because we need to know its class
name for code generation purposes).

I also wonder if you can demonstrate the performance impact of generating
proxies over simply injecting the required object -- particularly against
using the approaches I outlined.

Good point. I will surely do!

(Maybe I'm being thick, but I'm simply not grasping the concept; if I
cannot, how can we expect consumers of the framework to understand and use
it?)

This is new stuff to PHP. Generated Proxies are well known by frequent
users of Doctrine, but we introduced a lot of new stuff in the last years.
This is one of these things :) I'm confident that it can be documented and
explained quite well. I'll have to document it for doctrine/common too, and
surely for ZF2 too if this PoC is accepted and I am allowed to work further
on it.

Marco Pivetta

http://twitter.com/Ocramius

http://ocramius.github.com/

Contributor

lsmith77 commented Nov 20, 2012

@weierophinney in simple terms all this does it wrap the original service in a dynamically generated proxy class, that extends the class of the service, has the service locator injected along with the service name and then lazy loads that service as soon as you start interacting with the service.

this concept was introduced in Doctrine as one of the many possible lazy loading mechanisms when dealing with Entity references and its simply being applied here to deal with services in a DIC.

Member

Ocramius commented Nov 20, 2012

@weierophinney thought a bit about the problem of "splitting controllers", which basically means to decompose each action so that you don't have unused dependencies.

That's already not possible with a lot of restful controllers in my personal case, since I would be separating problems that belong all together.

Again, I think this patch is mainly to stop the service location madness that is going on, and which is FAR from what your ideal explained use case (that I agree with). It's about giving users a transparent performance tweak without having them write dirt in their classes.

I also added an extensive explanation of the problem at http://ocramius.github.com/blog/zf2-and-symfony-service-proxies-with-doctrine-proxies/

Owner

weierophinney commented Nov 20, 2012

@Ocramius since you're clearly generating proxies, is there a way to cache the generated classes?

Member

Ocramius commented Nov 20, 2012

@weierophinney it only works via file_put_contents/require (you can notice an autoloader I've added to the factory included in this PR). That obviously because we need to use opcode caches.

I use caching only for the proxy FQCN (based on requested service name).

Member

ralphschindler commented Nov 26, 2012

The only requirement here would be https://github.com/Ocramius/zf2/blob/8b58ed45fa09c413558c1ad88db8c3ecefaef5bc/library/Zend/ServiceManager/ServiceManager.php#L492-L499, which basically means making it possible to define a factory that handles service proxy (or generic lazy services) instantiation.

Why is even this needed if you are building this as a module? Couldn't your module simply replace the ServiceManager with a ServiceManager proxy that does all the smart generation at create() time, and also doing the call forwarding to the real ServiceManager?

The part of the code that aims to confuse me is that the proxy targeted services are called "lazy services", which is an odd choice of terminology since a service manager by its very nature is lazy loading by default.

Contributor

lsmith77 commented Nov 26, 2012

well its lazy loading .. but the services themselves might not be lazy in terms of their dependencies or setup work done when making an instance. but i see your point .. dont have a better name to suggest as an alternative though.

Member

Ocramius commented Nov 26, 2012

I don't have a better naming either. LazyObject is fine too, I don't really care :)

(as a side note, if we really want to handle this as "lazy-something", then I'd argue that the component itself should be an ObjectFactory - we don't really want that I guess)

Member

Ocramius commented Nov 27, 2012

I will try and move this problem to an external module.

@Ocramius Ocramius closed this Nov 27, 2012

Member

Thinkscape commented Nov 27, 2012

:\

That means further destandarization ...

  __
 /.)\   +48 695 600 936
 \(./   abodera@gmail.com

On Tue, Nov 27, 2012 at 9:56 AM, Marco Pivetta notifications@github.comwrote:

I will try and move this problem to an external module.


Reply to this email directly or view it on GitHubhttps://github.com/zendframework/zf2/pull/2995#issuecomment-10750292.

Contributor

lsmith77 commented Nov 27, 2012

yes and no .. no in the sense that this solution will just require someone to change the service configuration, there is no need to change code. yes in the sense that people might favor coming up with their own solutions to solve performance issues because they might be unaware of the module and/or prefer to not make a hard dependency.

Member

Ocramius commented Nov 27, 2012

@Thinkscape probably. I'll try to do something robust and well tested.

Member

ralphschindler commented Nov 27, 2012

There's a few things going on here, and more than 1/2 of it is centered on a "philosophical" debate about SL vs DI (I don't ever anticipate this ever ending btw), but that is not worth addressing here.

One of the assertions is about performance, but the problem with this argument is that the infrastructure in and of itself is not inperformant, but the application code. The current argument is that either controllers or service layer instances might have particular workflows that don't consume all of their dependencies. @weierophinney's argument is sound in that if you're worried about a consuming object not fully consuming all dependencies, then perhaps you would re-work or refactor those instances. I think that is a fair argument as well.

Fact of the matter is that there are a few features needed to round out the service location ecosystem, features that were planned but never implemented that could potentially help developers. Some of these depend on a console or debug environment being available (which at 2.0.0 time, there was none). We should finish these, it will make understanding which modules consume and provide services, their names, and their types more evident.

I would love to see this fleshed out as a module first, as well as better arguments about what kind of normal classes are impacted by having to consume expensive unused dependencies. In a gist with @Ocramius yesterday, I showed him how this was possible with the current MVC stack as well as the stock ServiceManager (https://gist.github.com/4151184).

@Ocramius Ocramius referenced this pull request in Ocramius/OcraServiceManager Nov 28, 2012

Closed

Implement service proxies #1

Member

Ocramius commented Dec 2, 2012

For anyone still interested in this one, a working module is available at https://github.com/Ocramius/OcraServiceManager

The performance test suite reports:


string(32) "Instantiating 1000 base objects:"
double(0.22778558731079)
string(49) "Instantiating 1000 service locator aware objects:"
double(0.25711679458618)
string(33) "Instantiating 1000 proxy objects:"
double(0.23988699913025)


string(31) "Initializing 1000 base objects:"
double(0.029274702072144)
string(48) "Initializing 1000 service locator aware objects:"
double(0.45500326156616)
string(32) "Initializing 1000 proxy objects:"
double(0.33832550048828)


string(29) "Calling on 1000 base objects:"
double(0.029347896575928)
string(58) "Calling on 1000 initialized service locator aware objects:"
double(0.057409524917603)
string(42) "Calling on 1000 initialized proxy objects:"
double(0.055790424346924)

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