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

compiled interceptors module #22826

Open
wants to merge 41 commits into
base: 2.3-develop
from

Conversation

@fsw
Copy link
Contributor

commented May 10, 2019

Description (*)

At Creatuity we have developed a module that generates Interceptors code using app config instead of a boilerplate that reads config at runtime.

https://github.com/creatuity/magento2-interceptors

We believe it changes performance and debugging magento significantly and it is worth to consider making it a part of the core witch this pull request does.

Any comments welcome.

Manual testing scenarios (*)

to use in developer mode in app/etc/di.xml replace:

<item name="interceptor" xsi:type="string">\Magento\Framework\Interception\Code\Generator\Interceptor</item>

with:

<item name="interceptor" xsi:type="string">\Magento\Framework\CompiledInterception\Generator\CompiledInterceptor</item>

clear generated files and cache:

rm -rf generated/* && rm -rf var/cache/* && bin/magento cache:clean

generated interceptors instead of boilerplate that reads plugins config at runtime like this:

public function methodX($arg) {
    $pluginInfo = $this->pluginList->getNext($this->subjectType, 'methodX');
    if (!$pluginInfo) {
        return parent::methodX($arg);
    } else {
        return $this->___callPlugins('methodX', func_get_args(), $pluginInfo);
    }
}

should have a code built using config like:

public function methodX($arg) {
    switch(getCurrentScope()){
        case 'frontend':
            $this->_get_example_plugin()->beforeMethodX($this, $arg);
            $this->_get_another_plugin()->beforeMethodX($this, $arg);
            $result = $this->_get_around_plugin()->aroundMethodX($this, function($arg){
                return parent::methodX($arg);
            });
            return $this->_get_after_plugin()->afterMethodX($this, $result);
        case 'adminhtml':
            // ...
        default:
            return parent::methodX($arg);
    }
}

Contribution checklist (*)

  • [*] Pull request has a meaningful description of its purpose
  • [*] All commits are accompanied by meaningful commit messages
  • [*] All new or changed code is covered with unit/integration tests (if applicable)
  • All automated tests passed successfully (all builds on Travis CI are green)

fsw added some commits Jan 7, 2019

@m2-assistant

This comment has been minimized.

Copy link

commented May 10, 2019

Hi @fsw. Thank you for your contribution
Here is some useful tips how you can test your changes using Magento test environment.
Add the comment under your pull request to deploy test or vanilla Magento instance:

  • @magento-engcom-team give me test instance - deploy test instance based on PR changes
  • @magento-engcom-team give me 2.3-develop instance - deploy vanilla Magento instance

For more details, please, review the Magento Contributor Assistant documentation

@magento-cicd2

This comment has been minimized.

Copy link
Contributor

commented May 10, 2019

CLA assistant check
All committers have signed the CLA.

@orlangur

This comment has been minimized.

Copy link
Contributor

commented May 11, 2019

Hi @fsw,

We believe it changes performance and debugging magento significantly

Do you have some benchmarks to share with us?

@IvanChepurnyi

This comment has been minimized.

Copy link

commented May 11, 2019

@fsw This is a very nice module, do but did you consider compiling interceptors per area and specifying those on di for Magento to use, instead of using static calls to "getCurrentScope"?

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 11, 2019

@orlangur, on a standard instance, simple ApacheBench tests are showing about ~10% gain on a request, I will create some more detailed benchmark for you.

@IvanChepurnyi, actually this was the original idea, but:

  • we wanted this to be 100% backward compatible and there were issues with class per scope approach: scopes can be switched at runtime when interception should be working, compiled mode classmap would have to be hacked to use different class depending on scope etc. So it was simply easier to implement this way.
  • in a lot of cases plugins are exactly the same for all scopes so they are being grouped in a single switch case so we end up having 6 times less classes and code in opcache in exchange for this one static switch.

However I and not 100% sure used approach is better, do you think there are other upsides other than removing the static switch?

@IvanChepurnyi

This comment has been minimized.

Copy link

commented May 11, 2019

@fsw switch opcache argument sounds reasonable, but maybe you somehow could receive this current scope without doing this static call, like maybe pass it in the constructor, so DI container will automatically supply it for you?

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 12, 2019

@IvanChepurnyi this should be easily doable as code generator uses reflection anyway so should be easy to override parent constructor. I will check how this would affect performance.

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@orlangur, trivial benchmark in developer mode:

bin/magento cache:clean > /dev/null
rm -rf generated/* && rm -rf var/cache/*
echo "warmup"
curl -so /dev/null -w '%{time_total}\n' $url
echo "cached"
ab -n 100 -c 4 $url | grep "Time per request:"
request Interceptor (ms) CompiledInterceptor (ms) gain
home page warmup request 15555 31342 -101.49%
  time per request 176.439 166.831 5.45%
  time per request (4 concurrent requests) 44.11 41.708 5.45%
listing warmup request 14212 30557 -115.01%
  time per request 1813.871 1661.836 8.38%
  time per request (4 concurrent requests) 453.468 415.459 8.38%
product page warmup request 15720 32143 -104.47%
  time per request 177.388 172.602 2.70%
  time per request (4 concurrent requests) 44.347 43.15 2.70%
admin login page warmup request 16730 31671 -89.31%
  time per request 439.592 422.958 3.78%
  time per request (4 concurrent requests) 109.898 105.74 3.78%

You can see that first request with generated code purged renders a lot slower as interceptors now read config to generate, but in exchange following requests are faster. This obviously depends on number of plugins and should be better in production mode where DI takes far less time.

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@IvanChepurnyi I have followed your suggestion and it seems to be a bit faster and code is much nicer indeed.
examples of generated interceptors:

https://github.com/fsw/magento2/tree/2.3-develop/lib/internal/Magento/Framework/CompiledInterception/Test/Unit/CompiledInterceptor/_out_interceptors

@IvanChepurnyi

This comment has been minimized.

Copy link

commented May 13, 2019

@fsw Looks nice! But I just noticed, seems like you also use object manager directly for plugin instantiation, so maybe it makes sense to supply it as well as a dependency?

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 13, 2019

@IvanChepurnyi already tried that :) but:

  1. this would not be 100% backward compatible as currently plugins are "lazy loaded" as well and we can not be sure what happens in constructors. Here, we are doing stuff exactly in same order.
  2. when I tried that (passing plugins in constructor) I got a lot of "circular dependency" issues as plugins were depending on objects that were intercepted and depended on other plugins etc.
@IvanChepurnyi

This comment has been minimized.

Copy link

commented May 14, 2019

@fsw I mean passing object manager for instantiation of plugins later. Sure for performance reasons pre-instantiation of plugins does not make sense.

@kandy

This comment has been minimized.

I believe that it is possible to use a standard name for variables, they are private and cannot intersect with another variables.

This comment has been minimized.

Copy link
Contributor Author

replied May 14, 2019

unfortunately it wont work when parent class would have same properties declared as public/protected

This comment has been minimized.

Copy link
Contributor

replied May 14, 2019

yes, you're right

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 14, 2019

@IvanChepurnyi oh yes, that makes sense. done.

Also I have 2 questions for general public:

  1. Is slower generation time on first request a big problem? If so we can think about modifying class loader somehow so it would check file mtimes to cover most cases so developer would not have to purge var/generated/* each time plugin is added.

  2. Is adding this as a separate generator with an option for a fallback a good approach? Because if this is 99.99% compatible (except edge cases like someone having plugins on config loaders) other approach would be to replace default interceptor generator with this?

@IvanChepurnyi

This comment has been minimized.

Copy link

commented May 14, 2019

@kandy good point.
@fsw how about creating a file watcher command line tool that will automatically clear generated code if plugin for that class is added in di.xml? I think it will greatly increase DevExperience, but I think it can be done a separate tool outside of this pull request, as it requires a dev to install an inotify extension, but it is worth it. Also, it is possible to use output from this tool instead of notifying, as it is much faster: https://docs.rs/crate/unison-fsmonitor/0.1.0

@kandy

This comment has been minimized.

Copy link
Contributor

commented May 15, 2019

I think it makes sense to include your code directly to Interceptor library.
Also, we can implement a proxy that will select one of implementation depending on the mode. It may answer your question.

Also if this code will work in production mode we can make additional optimization like replace

$this->____om->get(\Magento\Framework\CompiledInterception\Test\Unit\Custom\Module\Model\ItemPlugin\Advanced::class);

with

new \Magento\Framework\ClassReplacedByPrreferance(
 $this->____om->get(\Dep1),
 $this->____om->get(\De2),
 $this->____om->get(\Dep3),
)
@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 16, 2019

@kandy I think plugin instances are shared between interceptors, plus I think there is a bit more logic in DI when it creates constructor arguments (constants/virtual types etc) so I am not sure if removing this single step of DI would be worth it.

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

Changes to interceptor constructors (adding objects with DI) were fixed in compiled mode.
One extra preference needs to be added for it to work.

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented May 30, 2019

@joni-jones @maghamed all your remarks from your reviews were either fixed or require further discussion. Not sure if I shall take any steps to push those back to you.

@davidverholen

This comment has been minimized.

Copy link
Member

commented Jun 5, 2019

@fsw is this commit a fix for this issue ?

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 6, 2019

@davidverholen

This issue is with the default method and I was not aware it exists.
This commit fixes general issues with methods returning "self" so it fixes this #11905 for compiled mode.
However I am using getDeclaringClass to substitute for self so probably #22769 wont apply in compiled interceptors and this bit:

    if ($returnTypeValue === 'self') {
        $returnTypeValue = $method->getDeclaringClass()->getName();
    }

can be also used to fix #22769 in default mode.

@maghamed

This comment has been minimized.

Copy link
Contributor

commented Jun 6, 2019

@fsw
The corresponding PR with Architectural proposal is created - magento/architecture#185

Currently, I run voting regarding the incorporation of this PR in #appdesign Slack channel, please join there to Vote
https://magentocommeng.slack.com/archives/CBSL1DF8B/p1559858324023000

@joshuaadickerson

This comment has been minimized.

Copy link

commented Jun 7, 2019

Use type hints.

@fsw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 7, 2019

I have pushed a fix to optimize config loading and memory usage to improve setup:di:compile performance as it was affected as pointed out by @joni-jones

compilation timing:

without module: 2m39.824s

with module before fix: 5m47.604s

with module after fix: 2m33.752s

Also I have discovered that warmup request takes so long only because we generate code for all scopes at once. Hence when you clear generated and go to fronted, even admin config XMLs are red/parsed etc.

I think that if it makes such a significant difference maybe it will be worth reconsidering the idea of writing a separate interceptor class for each scope. I did not like the idea at first because I am not sure how hard would it be and how it will affect the system to modify OM to load different classes depending on scope.

Another solution would be to have a 'intermidiate' Interceptor class that calls FrontendInterceptor or AdminInterceptor class. Not sure how this will affect code readability though.

fsw added some commits Jun 10, 2019

Merge pull request #6 from magento/2.3-develop
merge upstream changes
@fsw

This comment has been minimized.

Copy link
Contributor Author

commented Jun 10, 2019

Further investigation showed that the issue is not exactly reading config for all scopes but the fact that PluginList does not cache data at scope level hence:

  • generator was loading config for general|frontend
  • *.xml files for general and for frontend scopes were loaded and merged
  • generator was loading config for general|adminhtml
  • *.xml files for general and for adminhtml scopes were loaded and merged
  • and so on
    Hence general scope has lots of xml parsing them for each scope was the bottleneck.

Last commit fixed this issue. Here is the updated benchmark:

reqest   Interceptor CompiledInterceptor gain
home page warmup request 18498 18383 0.62%
  time per request 217.1 207.97 4.21%
  time per request (4 concurrent requests) 54.275 51.993 4.20%
listing warmup request 18747 18206 2.89%
  time per request 2878.525 2727.68 5.24%
  time per request (4 concurrent requests) 719.631 681.92 5.24%
product page warmup request 18833 18784 0.26%
  time per request 217.022 206.669 4.77%
  time per request (4 concurrent requests) 54.255 51.667 4.77%
admin dashboard page warmup request 22955 20965 8.67%
  time per request 1363.842 1201.763 11.88%
  time per request (4 concurrent requests) 340.96 300.441 11.88%

@maghamed I guess as slower first request time was the biggest downside here and now it is even a bit faster, the poll should be rebooted with different question? Sorry for the confusion, I was under the wrong impression reflection is the bottleneck here.

@amenk

This comment has been minimized.

Copy link
Contributor

commented Jun 22, 2019

@fsw Thanks for the awesome work. I can't wait to get this merged and I hope it will (I am a big fan of the improvements in the debugging process)

Can you fix the static tests to get everything green?

fsw added some commits Jun 24, 2019

Merge pull request #8 from joni-jones/interceptors
Replaced preferences usage

@fsw fsw referenced this pull request Jun 24, 2019

Merged

Replaced preferences usage #2

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.