Pointcut compilation #346
Pointcut compilation #346
Conversation
| return new PointcutRegexExpression($token); | ||
| } | ||
| if (TypeDescriptor::isItTypeOfExistingClassOrInterface($token)) { | ||
| $classDefinition = ClassDefinition::createFor(TypeDescriptor::create($token)); |
There was a problem hiding this comment.
In the actual implementation, the interface registry is used here.
There was a problem hiding this comment.
What about using InterfaceToCallRegistry::getClassDefinitionFor?
From the PR I understood that intention to not use this is related to code visibility.
But ClassDefinition::createFor is not cached, therefore it will be resolved separately per each Pointcut.
There was a problem hiding this comment.
Because the compilation is done during pointcut creation (Pointcut::createWith) and the registry is needed during compilation.
I had 3 possibilities:
- no cahing :)
- injecting InterfaceToCallRegistry on
Pointcut::createWithbut not sure it is always available, and it is called in a lot of places, I did not want to pollute the review of the PR. - delaying compilation on first call to
Pointcut::doesItCut, but in that case invalid pointcuts can be created without throwing and I have to change some tests.
What do you think ?
There was a problem hiding this comment.
It would be interesting to have the registry at construction time also here:
There was a problem hiding this comment.
injecting InterfaceToCallRegistry on Pointcut::createWith but not sure it is always available, and it is called in a lot of places, I did not want to pollute the review of the PR.
This is how we do it with other components, but I see your point about amount of extra work.
Anyways not a blocker for this one, as seems that this PR actually bring performance improvement even without caching.
Let's go without the cache then :)
There was a problem hiding this comment.
It is ok for the amount of extra work, it was for clarity for the reviewer :) I may add this in another PR.
packages/Ecotone/src/Messaging/Handler/Processor/MethodInvoker/PointcutParser.php
Outdated
Show resolved
Hide resolved
| return new PointcutRegexExpression($token); | ||
| } | ||
| if (TypeDescriptor::isItTypeOfExistingClassOrInterface($token)) { | ||
| $classDefinition = ClassDefinition::createFor(TypeDescriptor::create($token)); |
There was a problem hiding this comment.
What about using InterfaceToCallRegistry::getClassDefinitionFor?
From the PR I understood that intention to not use this is related to code visibility.
But ClassDefinition::createFor is not cached, therefore it will be resolved separately per each Pointcut.
packages/Ecotone/src/Messaging/Handler/Processor/MethodInvoker/Pointcut.php
Show resolved
Hide resolved
packages/Ecotone/src/Messaging/Handler/Processor/MethodInvoker/PointcutExpression.php
Show resolved
Hide resolved
packages/Ecotone/src/Messaging/Config/Annotation/ModuleConfiguration/BasicMessagingModule.php
Show resolved
Hide resolved
Description
Compile pointcuts during compilation phase.
Pointcuts are tested on a lot of different classes during compilation phase, so this PR may speed compilation phase of large projects.
For a medium sized project with Symfony, I obtained merely 10% performance boost on cache:clear.
(also made some cleaning, droping the dependency on
InterfaceToCallRegistryfor pointcuts. This could have a bad impact on performance as no reflection cache is used, but it required adding the registry to each MethodInterceptor::create calls)Type of change