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

RFC: Lazy instantiation and unification of reflection classes in MethodMetadata and PropertyMetadata #77

Closed
ThomasNunninger opened this issue Feb 19, 2019 · 7 comments

Comments

@ThomasNunninger
Copy link

Q A
Bug report? no
Feature request? yes
BC Break report? no
RFC? yes

As far as I see it, there is an inconsistency in MethodMetadata and PropertyMetadata:

  • MethodMetadata::__construct() (and ::unserialize()) instantiate a ReflectionMethod. Additionally the class offeres an invoke() method

  • PropertyMetadata does not have such behavior and does not create a reflection class.

I see two problems:

  • The behavior is inconsistent
  • On each unserialize of a class, a ReflectionMethod is created for each method. This costs time.

Proposal

I assume, it would have been best not to provide the $reflection property in MethodMetadata at all. But you can't remove it for now as that would be a BC break. Therefore I propose:

  • Lazy-initialize a ReflectionMethod only if needed and save it internally for later reuse.
  • Make MethodMetadata->reflection private and provide a __get() that handles this case.
  • Implement similar behavior for PropertyMetadata.

Questions

  • Is such a change desired?
  • Would you accept a pull request?
@ThomasNunninger ThomasNunninger changed the title RFC: Lazy instantiation and unification of reflection classes RFC: Lazy instantiation and unification of reflection classes in MethodMetadata and PropertyMetadata Feb 19, 2019
@goetas
Copy link
Collaborator

goetas commented Feb 19, 2019

That could be an interesting thing to do.

Are you using MethodMetadata into something particular? (jms/serializer uses it in really few cases)

@ThomasNunninger
Copy link
Author

For now, I'm more interested in PropertyMetadata. It was more that I disliked that the ReflectionMethod was instantiated for each method without any need.

Perhaps I could even omit creating the MethodMetadata for my ClassMetadata. But I offer to provide a patch anyway if desired.

@goetas
Copy link
Collaborator

goetas commented Feb 20, 2019

  • Is such a change desired?

if is a performance improvement, I would say yes. I have removed reflection from the other metadata type for exactly the same reason.

  • Would you accept a pull request?

If is backward compatible, yes.

@ThomasNunninger
Copy link
Author

Ok, I will provide a pull request within the next days.

One question: My proposal comprises two parts:

Peformance improvement of ReflectionMethod:

  • Lazy-initialize a ReflectionMethod only if needed and save it internally for later reuse.
  • Make MethodMetadata->reflection private and provide a __get() that handles this case.

Consistent behavior:

  • PropertyMetadata
  • ClassMetadata (I've forgotten about that before)

Are both parts desired? I'm only interested in the performance improvement. But I can provide the consistent behavior as well.

@goetas
Copy link
Collaborator

goetas commented Feb 20, 2019

PropertyMetadata and ClassMetadata starting from 2.0 do not initialize refelction on construction anymore

@ThomasNunninger
Copy link
Author

Sorry, I've forgotten to comment on that issue.

The pull request handles the performance improvement ensuring backward-compatibility, but does not re-introduce the reflection property to PropertyMetadata or ClassMetadata.

@goetas
Copy link
Collaborator

goetas commented Apr 21, 2019

closed via #78

@goetas goetas closed this as completed Apr 21, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

2 participants