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

AnnotationBasedAutowiring with additional options for performance tweaks #662

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
3 participants
@falkenhawk
Copy link

commented May 31, 2019

  • adds possibility to disable features which are not used
  • useAnnotations(boolean) toggle on autowire() helper to enable/disable reading annotations on specific definitions
  • default behaviour does not change (backwards compatibility)

Available flags for $builder->useAnnotations(true, $flags):

  • AnnotationBasedAutowiring::ALL - all annotations enabled (default)
  • AnnotationBasedAutowiring::IMPLICIT - enable on implicit definitions (not specified in container config)
  • AnnotationBasedAutowiring::EXPLICIT - enable on all autowire definitions (which are written in DI config) by default
  • AnnotationBasedAutowiring::INJECTABLE - read @Injectable annotations for classes
  • AnnotationBasedAutowiring::PROPERTIES - read @Inject annotations for properties
  • AnnotationBasedAutowiring::METHODS - read @Inject annotations for methods' parameters

We are using the following combination of options:

$builder = new DI\ContainerBuilder();
$builder->useAnnotations(true, AnnotationBasedAutowiring::IMPLICIT | AnnotationBasedAutowiring::PROPERTIES);

It that case it means that:

  1. Annotations will be read when a class is not explicitly defined in definitions file
  2. annotations will be disabled by default for classes defined in definitions file, autowire()->useAnnotations() to enable annotations
  3. @Injectable is skipped for performance
  4. @Inject on properties will be read only when annotations are enabled for an entry (either on implicit definitions or explicit with useAnnotations())
  5. @Inject on methods will be skipped for performance - as we don't use them

[from ovos#4]

AnnotationBasedAutowiring with additional options for performance tweaks
- adds possibility to disable features which are not used
- useAnnotations(boolean) toggle on autowire() helper to enable/disable reading annotations on specific definitions
@SvenRtbg

This comment has been minimized.

Copy link

commented May 31, 2019

There are no tests for that feature. And you should probably back your claim of performance gain with some benchmark tests as well.

@mnapoli

This comment has been minimized.

Copy link
Member

commented Jun 1, 2019

And you should probably back your claim of performance gain with some benchmark tests as well.

@SvenRtbg I am the one supposed to do the benchmarks, per our previous discussion with OP.

@falkenhawk thanks for opening the PR, I will review them over the coming days (possibly weeks) ;)

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.