Proxies of module interfaces don't work

Wow :(

I'm not sure this is supposed to work... oh php is fun :)

Maybe exploring #2354475: [meta] Refactor the installer, (multi)site management, and pre-container bootstrap will lead to a solution.

I should note that the patch in #3 actually seems to solve the issue fairly well. The only failing tests (as far as I can see) are the ones which are concerned with the output of the proxy-builder component, and can probably be fixed easily. I have been meaning to do that, but just haven't found the time...

#3 looks great to me and will improve performance, too!

Fixed the tests.

We need to ensure an opcode cache is used for the classes still, so need some profiling.

Pretty sure the opcode for the classes themselves won't be cacheable, at least according to http://marc.info/?l=pear-dev&m=118365656701421&w=2

I'd still expect this to be better than loading all the interfaces, but not sure how well we can measure this.

I'm bumping this to critical since by marking the Update module's services as lazy we can save over 1mb per admin page get and thousands of function calls.

Profiling the user permissions page as user 1...

I think the above performance gains and how this will allow contrib to enjoy the same performance as core makes this critical.

Fixed the ConfigImporter ... SimpletestTest and test inception is proving more difficult.

Repeated profiling from #12... slightly less improvement in function calls... caches must not have been entirely warmed but the memory improvement is still significant.

Overall: +1

I am not sure the function approach gives us op-code cached classes. If it does => Great, but if not we might introduce a performance regression for after-page-cache.

That we would need to test in isolation.

Also:

My opinion is proxies should be:

a) optional
b) autoloaded from \Drupal\[module]\DrupalLazyProxy\[NS]\[ClassName]

and statically created for each module and committed to GIT.

If the proxy class cannot be loaded, fall back to the real service, which is possible.

That would make:

a) the container lighter
b) ensure we don't cache information that is purely static usually

--

Allow a contrib author to just do e.g.:

"lazy: true"
drush build-lazy-proxy

or

"lazy: true"
./core/scripts/build-lazy-proxy.sh [directory]

I think that is better than adding more and more information into the container.

Why are update.module's services even being initialized on admin pages? I don't understand that part yet.

Is this going to be command line only? Do we have any other feature requiring command line? If this is a viable why dont we move rebuilding itself to command line? I do not fully understand the repercussions and target users here.

@chx we discussed checking the proxies into git - so this would be more like a subset of 'module builder' than a rebuild operation.

Looks like a great start.

Overall looks really really awesome!

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php
   @@ -0,0 +1,33 @@
   +        $proxy_class = ProxyBuilder::buildProxyClassName($definition->getClass());
   +        if (class_exists($proxy_class)) {
   +          $definition->setClass($proxy_class);
   +        }
   

   I don't think that will work for factories or such.

   Should this not do:

   - Move the real definition to 'drupal_proxied.[service_name]'

   - Add a new definition with just the class (as that is all the proxy should care about).

   - Have the proxy get drupal_proxied.service_name instead.

   ( or drupal_proxy.real_service.[service_name])

   I was never a fan of re-using the same definition twice as accessing the getX methods is kinda a hidden implementation detail.

   The advantage:

   - Factories, special cases, etc. all work out of the box.

   => Nice isolation.

   => Could give the real service name as an argument to the proxy, so its transparent.

   ( => Fallback theoretically possible if proxy class cannot be found.)

2. +++ b/core/lib/Drupal/Core/ProxyClass/Extension/ModuleInstaller.php
   @@ -0,0 +1,79 @@
   +            if (!isset($this->service)) {
   +                $method_name = 'get' . \Symfony\Component\DependencyInjection\Container::camelize($this->serviceId) . 'Service';
   +                $this->service = $this->container->$method_name(false);
   +            }
   

   This is an implementation detail of the container and not public API.

   E.g. it would break with my container ... ;)

   Should just use:

   $this->container->get($serviceId);

   instead.

   With moving the proxied class to another service name namespace (see above) this works well and with using that new namespace as an argument to the proxy, the proxies are independent of any implementation. They just know how to get the real service, regardless where it lives.

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php
   @@ -20,11 +21,20 @@ class ProxyServicesPass implements CompilerPassInterface {
            $proxy_class = ProxyBuilder::buildProxyClassName($definition->getClass());
            if (class_exists($proxy_class)) {
   

   I think we should move that check up to before creating the proxied service definition?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php
   @@ -20,11 +21,20 @@ class ProxyServicesPass implements CompilerPassInterface {
   +          $proxy_definition = new Definition($proxy_class);
   +          $proxy_definition->setArguments(['service_container', $new_service_id]);
   

   Uh, don't we need to register the proxy definition instead of the real definition?

   Also:

   'service_container' won't fly needs to be:

   new Reference('service_container')

   if my container foo is strong enough today :D.

Overall looks really really great!

Some questions:

1. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php
   @@ -0,0 +1,44 @@
   +        $proxy_class = ProxyBuilder::buildProxyClassName($definition->getClass());
   +        if (class_exists($proxy_class)) {
   

   I wondered a little what is better:

   a) fail silently as done here

   b) throw an Exception with the message to run the script for this file.

   After all you always have a choice to not mark things lazy and because now every module is responsible and hence "lazy: true" is matched to the module itself (that ships the proxy), it might be okay to throw an Exception.

   Not 100% sure though. Thoughts?

2. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php
   @@ -0,0 +1,44 @@
   +          $container->register($service_id, $proxy_class)
   +            ->setArguments([new Reference('service_container'), $new_service_id])
   +            ->setMethodCalls($definition->getMethodCalls());
   

   Oh, what is the use case for the setMethodCalls?

   Would that not lead to the real service being loaded when the 'calls' call a method on the proxy?

a) fail silently as done here

b) throw an Exception with the message to run the script for this file.

Looks like a good case for an assertion?

#41: Well, no because we ignore the lazy in case that the class does not exist.

I think:

- Exception now (helpful for core as seen) and being explicit
- @todo pointing to Assertions
- Use Assertion once they exist

Overall looks great.

Anything you miss on this, dawehner or can I go to final RTBC review?

The IS says:

Instead of generating the proxy classes on container rebuild time we build them using a custom script: generate-proxy-class.php>
and dump them into the file system, for example:

But it seems this patch is committing them? I'm confused now. We need an updated IS.

Yes, we need an IS update.

Anything actionable we can do on the test failure? Is this really random?

Added issue summary updates

Thanks, the IS is now 300% more clear.

Hm. I am very very nervous about the required pre-compile step. It feels like it's in the same class as forcing theme developers to use Sass and then regenerate CSS.

I don't follow why we can't continue to generate the proxy files at runtime, and put them in PHPStorage. I understand this issue makes that not happen, but I do not see how that's a requirement of fixing the root issue (that modules can't use proxies right now). That seems like a huge step back to me DX-wise to force module developers to generate (and remember to regenerate) their own proxies.

Also, advantage B is not true: The size of the container itself has virtually no impact on performance as long as the file source is small enough to not bust the opcode cache limit for a file; I don't even know what that limit is in PHP 5.5, but APC had a limit. :-) I benchmarked that back when we were debating how to handle injection into controllers and there was no detectable performance difference from the size of the generated container.

As some of you might remember, I've been a fan of generating things for a very, very long time. And it had tremendous performance promise #35657-15: split mode. We could've utilized it for unit testing. For a somewhat more recent development, see the Automatically generated entity classes per bundle for better IDE integration project -- as an aside, entity classes ought to be abstract and only these generated per bundle classes should be instantiatable.

But I can only agree with Crell in this: generating things on the fly by Drupal is a very different kettle of fish to pregenerating things (the paragraph above doesn't discern between them as it merely wanted to show I've been in this particular arena for a decade). I wrote phpstorage to make generating feasible. I feel we should utilize it a lot more (like entity classes per bundle should be core and everything else we can / want to crank performance) and not enforce command line scripts.

There are not that many lazy classes. Are we sure we can't do this on-the-fly?

Proxy services are a pretty special case.

The amount of effort is way less to add "lazy: true" and then run a script, than to e.g. write a block plugin from scratch. Therefore the effort vs. gain is clearly not a DX hinderance. You get a helpful exception or assertion if your proxy class is missing and we could add a check to load the proxy class (when assertions are in) to assert that the interface still matches (loading the class should be enough for that), or we could put a quick checksum on the interface.

Overall: No one forces you to use proxy services. It is mainly useful for core and core modules.

#57: I like PHP Storage, but we have long decided on many fronts that explicit is better than implicit, so why should we do work at run-time that can be done at compile time? Especially the MTimeProtectedFastFileStorage makes autoloading impossible atm.

--

Gonna take a look at the test failure today.

Yes we have #2497143: Proxy services in the container force interfaces to be loaded tracking the performance issue of proxies in the container. When the proxy class itself isn't going to be used at all on the request (especially internal page cache, but likely more and more things once SmartCache is enabled), then you get a performance hit of loading interfaces that will never be used. It could easily be dozens of classes if there's a lot of services marked lazy. It's currently 12-15 which is about 1/5 of the classes loaded on page cache hits.

So we need them in separate PHP files either way.

However as alexpott has pointed out, when the proxy class is instantiated, we have to load that code which we currently get nearly for 'free' since it's already in the container. PHPStorage has a runtime performance hit and we'll get that for every interface.

This is an entirely optional API, so it's nothing like SASS, it's more like minification of JS files. So I really lean towards pre-generation here.

One thing we could do is skip the exception and use trigger_error() instead when building the container at least until we have asset(). Then when developing, you get nagged each container rebuild that you need to build the proxies, but you can mark something lazy:true and only generate the class at the last minute (or potentially we can get d.o packaging to do that for contrib modules).

And...

We discussed this on the critical triage call.

This should not be a hard blocker to #2497243: Replace Symfony container with a Drupal one, stored in cache since it would be possible to commit that patch, have no functioning proxies for a while, then bring them back with this patch.

However that would mean introducing a performance regression. So if that patch is ready first, we should profile and see what the regression looks like without this one applied.

If that regression was not bad at all, we could even consider downgrading this to major and adding it back in a patch-level release (with the warning on missing proxies added in the next minor release), but need to bear in mind that as well as whatever the performance regression is, it also makes things harder profiling since you need to take the lack of proxies into account everywhere. Either way tagging for deferred triage and this looks close to ready anyway.

Another reason not to use PhpStorage here - this came up in discussing with dawehner in irc.

Currently I do rm -rf sites/default/files/php/* quite regularly, since things get rebuilt afterwards. You can also delete any individual file or directory within that folder and things still work.

If we write the proxy classes there on container rebuilds, then:

1. With current state of HEAD, removing the proxy class files but not the container files would mean a fatal error.

2. With the container not compiled to PHP, rm -rf sites/default/files/* will get you in the same position since that won't invalidate the container either.

Twig is not like this - operates more like a cache, as does the compiled container.

And last thing.

Adding the checksum seems optional and a major-follow-up to me. If we hash MyInterface.php then just changing a code comment will mean a new checksum - not what we want.

dawehner suggested it might be possible to hash the reflection object, but I think just expecting people to update the classes if they change the interface is reasonable. The whole point of interfaces is that they change very rarely - and you can just not mark a service lazy until your interface is finalized while iterating something in the first place.

Unassigning for a moment as dawehner and catch check the test failure, too.

Put some extra debug into setUp() and it's WebTestCase::setUp() that has the memory leak. Especially running the installer and installing static $modules, so this suggests a memory leak in module install somewhere. Not really further than that though.

Needing to use the dumper is unfortunate. That memory leak should be fixed though - regardless of the approach we take here.

I found the first memory leak:

MimeTypeGuesser::registerWithSymfonyGuesser($this->container);

does

$singleton = SymfonyMimeTypeGuesser::getInstance();
$singleton->register($container->get('file.mime_type.guesser'));

BUT that singleton never forgets (but it is a protected value (!) so we could add a reset value) and now it gets worse:

file.mime_type.guesser is a proxy, therefore we store all container (builder) instances ever created during a test run ...

However not even marking that non-lazy would help, because it also stores all other proxies internally, so then again the container is stored indefinitely ...

Besides that found:

- We always call addPsr4 instead of setPsr4, so we add more and more namespaces to the autoloader.
- registered stream wrappers leak very tiny amounts of memory (10k) that is not gained back by unregistering
- https://bugs.php.net/bug.php?id=67111 also leaks a little memory in Yaml::parseInline

Just patched Symfony for now, we can cleanly subclass though and do a reset by unsetting the protected property and calling __construct again or copying construct ...

This hopefully passes on test bot ...

Interdiff for #72

Here we go:

Fixes the memory leak by using ReflectionProperty.

Justification:

08:01 < Fabianx-screen> If they (Symfony) use private, I use Reflection ...
08:01 < Fabianx-screen> easy deal ;)
08:01 < Fabianx-screen> Or rather: if they use private + singleton + static that _cannot be reset_.

We can clean this up once Symfony has added a reset() method.

Hopefully should be green.

1. This is looking good. I'm a bit concerned about the code generation. The interface change the need to rebuild is going to cause problems.
2. Any reasons why the the block repository service from the original issue is not being marked as lazy?

3. +++ b/core/lib/Drupal/Component/ProxyBuilder/ProxyBuilder.php
   @@ -34,19 +57,32 @@ public static function buildProxyClassName($class_name) {
   -  public function build($class_name) {
   +  public function build($class_name, $proxy_class_name = '') {
   

   Missing @param for new argument

4. +++ b/core/lib/Drupal/Core/Command/GenerateProxyClassCommand.php
   @@ -0,0 +1,94 @@
   +    $this->setName('generate-proxy-class')
   +      ->setDefinition([
   +        new InputArgument('class_name', InputArgument::REQUIRED, 'The class to be proxied'),
   +        new InputArgument('namespace_root', InputArgument::REQUIRED, 'The filepath to the root of the namespace.'),
   +      ])
   +      ->setDescription('Dumps a generated proxy class into its appropriate namespace.');
   

   Can you use the ->addUsage() method to add some examples of how to use this for core/contrib/custom

5. +++ b/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php
   @@ -0,0 +1,49 @@
   +          // @todo Replace this with an assertion once
   +          //   https://www.drupal.org/node/2408013 is in.
   +          throw new InvalidArgumentException(sprintf('Missing proxy class %s for service %s', $proxy_class, $service_id));
   

   This could build the command to run I think?

6. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1180,7 +1179,6 @@ protected function dumpDrupalContainer(ContainerBuilder $container, $baseClass)
   -    $dumper->setProxyDumper(new ProxyDumper(new ProxyBuilder()));
   

   No need to use Drupal\Core\ProxyBuilder\ProxyBuilder anymore then.

7. +++ b/core/lib/Drupal/Core/DrupalKernel.php
   @@ -1305,7 +1303,7 @@ protected function classLoaderAddMultiplePsr4(array $namespaces = array()) {
   -      $this->classLoader->addPsr4($prefix . '\\', $paths);
   +      $this->classLoader->setPsr4($prefix . '\\', $paths);
   

   Should we test this somewhere?

8. +++ b/core/lib/Drupal/Core/File/MimeType/MimeTypeGuesser.php
   @@ -101,6 +101,20 @@ protected function sortGuessers() {
        $singleton = SymfonyMimeTypeGuesser::getInstance();
   +
   +    // @todo Remove once Symfony adds a reset() method.
   +    $property = new \ReflectionProperty(get_class($singleton), 'guessers');
   +    $property->setAccessible(TRUE);
   +
   +    if (isset($singleton->_beforeDrupalRegistration)) {
   +      // Reset state, else we store more and more services during test runs.
   +      $property->setValue($singleton, $singleton->_beforeDrupalRegistration);
   +    } else {
   +      // Store original state before we register our services.
   +      $singleton->_beforeDrupalRegistration = $property->getValue($singleton);
   +    }
   +
   +    //$singleton->reset();
   

   ouch... btw SafeMarkup is also not reset in the parent site between tests.

9. +++ b/core/lib/Drupal/Core/Installer/InstallerServiceProvider.php
   @@ -58,7 +58,10 @@ public function register(ContainerBuilder $container) {
   +    $definition->setClass('Drupal\Core\Installer\InstallerRouteBuilder')
   +      // The core router builder, but there is no reason here to be lazy, so
   +      // we don't need to ship with a custom proxy class.
   +      ->setLazy(FALSE);
   

   Maybe it would be best to just not do the lazy compiler pass...

10. +++ b/core/lib/Drupal/Core/ProxyClass/Batch/BatchStorage.php
    @@ -0,0 +1,83 @@
    +        /**
    +         * The service container.
    +         *
    +         * @var \Symfony\Component\DependencyInjection\ContainerInterface
    +         */
    +        protected $container;
    ...
    +
    +        protected function lazyLoadItself()
    

    Why do we bother with property docs and not function docs? Imo we should just add documentation to everything.

11. +++ b/core/lib/Drupal/Core/ProxyClass/File/MimeType/MimeTypeGuesser.php
    @@ -0,0 +1,73 @@
    +        /**
    +         * @var string
    +         */
    +        protected $serviceId;
    

    This clashes with the $_serviceId added to all objects in the container. When you look at these in services in a debugger and see serviceId and _serviceId something looks very wrong.

#77:

1. Opened #2513336: Consider checking checksum of interfaces during container building for proxy services, but catch said it is out of scope here.

2. No, reason, but lets do that in a follow-up to measure the impact. Opened #2513340: Make block.repository a lazy service for that.

3. @todo Will be fixed

4. Done, also opened #2513338: Make it easier to create proxies.

5. Yes, however the PSR-4 namespace is not easy to detect. I'll try how difficult it is.

6. @todo Will be fixed

7. @todo This change is actually out of scope here, opened #2513344: Use setPSR4() instead of addPSR4() to avoid making the PSR-4 list bigger and bigger in the autoloader during test runs for it and will revert here.

8. yeah :/

9. Yes, but that is not that easy to do.

10. @todo Will be fixed.

11. @todo Will be fixed. Yes, indeed. I tend to use drupalProxyOriginalServiceId as name, because that matches the container definition and is explicit enough.

Fixed all issues:

- interdiff.txt - all fixes
- interdiff-proxies.txt - all resulting proxy class changes, I used the new error message to re-generate the proxies. Worked like a charm :).

Plus full patch.

Changes here:

- Error messages now look like:

<em class="placeholder">User warning</em>: 
Missing proxy class 'Drupal\filter\ProxyClass\FilterUninstallValidator' for lazy service 'filter.uninstall_validator'.
Use the following command to generate the proxy class:
  php core/scripts/generate-proxy-class.php 'Drupal\filter\FilterUninstallValidator' "core/modules/filter/src"

 in <em class="placeholder">Drupal\Core\DependencyInjection\Compiler\ProxyServicesPass-&gt;process()</em> (line <em class="placeholder">68</em> of <em class="placeholder">/var/www/html/core/lib/Drupal/Core/DependencyInjection/Compiler/ProxyServicesPass.php</em>).

- Added doxygen to all proxy methods.

- Fixed command to include real command how the proxy was generated

- Added addUsage() with examples for core, core modules and contrib

- Reverted out-of-scope addPsr4() change for now

Back to RTBC as only docs and error message changes

Test fixes

OK I've been following this closely and I think this is in a good state. We have the follow-up to add the interface hashing and to make proxy generation easier, change notice is enough for now though to resolve the critical. And allows us to mark #2497143: Proxy services in the container force interfaces to be loaded as duplicate.

Committed/pushed to 8.0.x, thanks!

Can we get a followup to add some document about lazy services to core.api.php - specifically the section on defining services in @defgroup container Services and Dependency Injection Container. Thanks

Opened #2526412: Remove Singleton hack in registerWithSymfonyGuesser to remove the hack. Thanks, dawehner!

Reopening for a question: is it intentional that the generate-proxy-class script that was added here only works for modules that are enabled?

I'd just been trying for 20 minutes to figure out what's wrong for a new module that was therefore not yet installed. Turns out this is what's wrong: I had not yet enabled the module… that I was trying to develop.

Re: #90 that sounds non-critical because it has a workaround. Let's open a non-critical followup.

Sorry.

Filed #2539494: Provide a nice error message when trying to run the generate proxy class script on uninstalled/missing modules for that.