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

Use entity listeners instead of event listeners #619

Closed
Koc opened this issue Sep 1, 2016 · 12 comments
Closed

Use entity listeners instead of event listeners #619

Koc opened this issue Sep 1, 2016 · 12 comments
Milestone

Comments

@Koc
Copy link
Contributor

Koc commented Sep 1, 2016

As I understand correctly - we create for each mupping N listeners. And all of this listeners called each time when any entity changes (even if it not mapped). Starting from Doctrine 2.4 there is entity listeners http://docs.doctrine-project.org/projects/doctrine-orm/en/latest/reference/events.html#entity-listeners which associated only with entity.

We can improve performance a lot by:

  1. get rid of unrelated listeners call - each entity has own specific listeners
  2. stop calling methods like isUploadable and getUploadableFields inside listeners because we already know which fields are uploadable for this listener
@Koc
Copy link
Contributor Author

Koc commented Sep 1, 2016

ref doctrine/DoctrineBundle#298

Koc added a commit to Koc/VichUploaderBundle that referenced this issue Sep 3, 2016
@Koc
Copy link
Contributor Author

Koc commented Sep 3, 2016

relates to #617

@Koc Koc mentioned this issue Nov 18, 2016
10 tasks
@garak garak modified the milestone: 2.0.0 Dec 16, 2016
@Koc
Copy link
Contributor Author

Koc commented May 7, 2017

Dear community, I need suggestion about implementing this feature. The problem is: we need get all mapped classes/properties inside compiler pass for creating listeners for each mapping. How it possible to do? I see 2 variants:

a. get some magic metadata reader service inside compiler pass like in propel compiler pass and read all metadatas for all classes
b. change configuration - get rid of Uploadable/UploadableField annotations and describe mapping inside config.yml

your suggestions?

// cc @garak @stof @schmittjoh @nicolas-grekas

@garak
Copy link
Collaborator

garak commented May 7, 2017

Option a looks better to me. Removing all annotations seems a big change, maybe too big.

@Koc
Copy link
Contributor Author

Koc commented May 7, 2017

Oher problem caused by option a - I'm not sure that know how to get all Doctrine's entities and it could be too expensive call. I need properly implement this method.

Maybe something like this https://abendstille.at/blog/?p=163 but it will slow down container recompilation. But we win in production.

@stof
Copy link
Contributor

stof commented May 8, 2017

I see 2 issues with options A:

  • it requires instantiating the Doctrine layer in a compiler pass. But instantiating any service inside a compiler pass is totally unreliable, as the container is not yet ready at this time (so it might work or break, you don't know. And it might depend of what other bundles are being used).
  • the Doctrine API to read all metadata is probably the worse Doctrine API in term of performance. The Doctrine metadata layer is optimized for getting the metadata for a given class, not for getting the list of classes for which it has metadata (the class name is always the cache key, so getting the list of classes cannot rely on the cache).

I suggest you to go with option b (which may even allow to make the configuration working the same for all persistence layers btw).

@stof
Copy link
Contributor

stof commented May 8, 2017

Btw, another option to improve performance is to register a single Doctrine event listener rather than multiple ones. This should be investigated too.

@Koc
Copy link
Contributor Author

Koc commented May 8, 2017

I suggest you to go with option b (which may even allow to make the configuration working the same for all persistence layers btw)

If we move mappings configuration to config.yml we can remove whole metadata layer. And also rewrite Propel compiler pass #636. Am I correct?

Btw, another option to improve performance is to register a single Doctrine event listener rather than multiple ones.

Did you mean single entity listener (which attached to concrete entity) or just single event listener (attached to all entities)?

@stof
Copy link
Contributor

stof commented May 9, 2017

no, I meant a single event listener, which will then know how to handle all uploadable entities (instead of having one global listener per entity). This may allow to keep using a single listener while keeping the existing configuration way.
You cannot use a single entity listener to manage all entities, as an entity listener is attached to a given entity (so you need one registration per uploadable class anyway)

Note that entity listeners are still using an event listener internally (a single one registered by the ORM, and filtering which entity listeners to call based on the entity being affected by the event).

@Koc
Copy link
Contributor Author

Koc commented May 10, 2017

@stof I'm trying this approach in #617. I've plans to use entity listeners for doctrine orm and single listeners for odm and phpcr. But looks like entity listeners is so hard to implement

@stof
Copy link
Contributor

stof commented May 10, 2017

why not using an event listener for the ORM too ? It may even allow to use the same listener implementation for all Doctrine projects (though registering different instances for each o*m)

@Koc
Copy link
Contributor Author

Koc commented May 10, 2017

ok, will move in this direction

@Koc Koc closed this as completed May 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants