Dispatching using own EventManager instance

[ghost]

Created by Andy Hobbs, 15th Sep 2013. (originally Lighthouse ticket #4059):

---

What I did

I tried to add a dispatch filter from a plugin bootstrap by doing the following:

  CakeEventManager::instance()->attach(function($event){  /* do something */}, 'Dispatcher.beforeDispatch', array('priority' => 11));

What happened

My event handler was called BEFORE the asset and cache filters

What I expected to happen

I expected my handler was called AFTER the asset and cache filters because I set a priority of 11

Because Dispatched, Controller, Model and View all use their own instances of the event manager by by calling "new CakeEventManager()" rather than "CakeEventManager::instance()" the priority setting is not having any effect as the global even manager is called first.

In my above example I ended up with 2 event managers which both handled the same event but not in the priority order requested.

If there is no reason to be using separate event managers rather than the global manager, then I've attached a patch for changing this.

[ghost]

11th Sep 2013, Andy Hobbs said:

---

My code patch didn't break any tests however it has broken my own code.

I have 2 models, one with behavior A and one with behavior B, now, with a shared event manager the beforeFind() callback for both behavior A and B get called for BOTH models. Obviously this is undesirable and so I see why Models need to have their own event manager.

However... Dispatcher, Controller and View classes probably also need their own event manager as I can think up scenarios where there will be more than one Controller or more than one View class (e.g. using requestAction()?).

Would it be possible to tie a "global" Event manager to the current request? This would then allow for the Dispatcher and Controller classes to use the global event manager.

The reason I want to continue to pursue this is that I think it is messy when using a plugin to have to start putting code into the main app to support the plugin. I always try and make my plugins autonomous as I use most of the plugins I write in multiple projects. I can't see any other way to

[ghost]

11th Sep 2013, Mark Story said:

---

The controller and view alredy share an event manager, primarily so components can hook into the view lifecycle.

An important thing to remember is that events attached to the global event manager are fired before instance level events are handled. They maintain a separate priority queue as they are separate instances. I don't see a simple way that global and local events could be mixed together, but if you have an idea that would be great.

How would the global event manager being tied to a request work with sub requests dispatched by requestAction?

[ghost]

12th Sep 2013, Andy Hobbs said:

---

Ok, so...

I had not read the complete docs page for Events. I stopped after the section describing event priority because I thought I had found what I needed so I didn't see lower down that the global event manager dispatches the event before the internal (obviously I did later find this in the code).

I think that mixing local and global events together should be straight forward. I'll have a go at it today and post my findings.

I've also thought a lot over night about how tying the global event manager to a request would work and I see that it would cause problems with requestAction rather than fix them. This has also raised lots of questions in my mind though about requestAction in general with any classes that have static properties, for instance using ClassRegistry::init() will cause problem if a model is modified within a requestAction call. But that's a different topic really.

[ghost]

12th Sep 2013, Andy Hobbs said:

---

Hi,

I've made my changes and added a test to demonstrate it. I've also had to update another test as I think that I encountered static functions under phpunit problems.

My commit is here:
https://github.com/andydhobbs/cakephp/compare/ticket-4059

Forgiv me if I've not done that right, this was my first time using git.

Andy

[ghost]

13th Sep 2013, Mark Story said:

---

It looks like a good start, I wonder what the impact on performance will be. The event system is a frequently used method so we need to keep it as performant as possible.

Moving to 2.5 as this will be a behavior change that might require people to change their applications.

[ghost]

13th Sep 2013, Andy Hobbs said:

---

I spent a few hours profiling this today and it was dispatching events 27% slower:

New Method: 4.53944 seconds to handle 100,000 events 
Old Method: 3.57186 seconds to handle 100,000 events 

I've been through a few different ways of doing the same thing and lots of minor tweaks to my original method and the code now runs faster, but not as fast as the original separate dispatching, still 12% slower:

New Method: 4.01390 seconds to handle 100,000 events 
Old Method: 3.57186 seconds to handle 100,000 events 

Latest code available here:
https://github.com/andydhobbs/cakephp/compare/ticket-4059
Changes since first version here:
andydhobbs@e1db5b4

[ghost]

13th Sep 2013, Andy Hobbs said:

---

I forgot to test it without global events registered (which I would have thought is the most common scenario?). Test results without global events registered:

It actually executes faster (6%)

New Method: 2.70600 seconds to dispatch 100,000 events
Old Method: 2.87823 seconds to dispatch 100,000 events

[ghost]

13th Sep 2013, Andy Hobbs said:

---

One more bit of optimisation and I have much better results:

This time I have run the tests in a controller action rather than a console shell, each time I have timed 100,000 iterations of dispatching an event using both the current and my new methods and also for local only and then local and global events listeners. The results show the number of seconds elapsed, captured using microtime().

For the local only tests there were 3 event listeners, priorities 5, 10 and 15.
For the local and global test there were 3 local listeners, priorities 5, 10, 15 and 1 global listener, priority 10

[old: local] => 9.3136820793152
[new: local] => 8.3860490322113 (10% faster)
[old: mixed] => 12.26331615448
[new: mixed] => 10.19167304039  (17% faster)

I did also do tests where there was no overlap between local and global tests: local listeners with priorities 5 and 15 with a global listener with priority 10:

[old: local] => 7.831463098526
[new: local] => 6.852735042572  (12.5% faster)
[old: mixed] => 10.658425092697
[new: mixed] => 8.5629329681396 (20% faster)

Both sets of results seem to indicate that the new method is faster.

There are scenarios I haven't tested which I am happy too add tests for, such as multiple local events with the same priority, multiple global events with the same priority etc. From the basic testing I have done I don't expect these extra scenarios to turn up very different results. If you feel it is desirable though I will spend more time on it?

[ghost]

14th Sep 2013, Mark Story said:

---

I think the changes are looking good, you might want to ensure that the + operator used on line 280 doesn't omit listeners as + acts oddly with arrays.

[ghost]

14th Sep 2013, Jose Lorenzo Rodríguez said:

---

I think there will be always a fight for when then global manager should be fired, when I did it, I purposefully made it so it fired before all other events and I could think a larger number of cases where having it dispatch before made more sense. I'm currently not sure changing the behavior of the dispatcher is a good idea as I have (and possibly plenty others) some applications that rely on the global event being dispatched first.

Can we make sure that by default events from the global manager with the same priority as local are dispatched first? If it is dependent on the attach order then I'm happy with this patch as it stands.

[ghost]

15th Sep 2013, Andy Hobbs said:

---

My change would have favoured local events over global events. I have changed it so that it now dispatches global events first and I've added another test case for this.

Mark: I used the + operator deliberately to maintain the array indexes as at this stage they are still the priorities and array_merge() would loose them. I played around with several different ways of doing this bit and all the other methods I came up with were slower than the original code.

Should I also make changes to the 2.x cookbook?

[ghost]

15th Sep 2013, Mark Story said:

---

Ok, a pull request on github is the best way to continue the change. The will need to be updated as well.

[ghost]

15th Sep 2013, Andy Hobbs said:

---

Hi,

I think I did this right:

#1637

I originally created my branch from "master" but I have issued the pull request against branch "2.5". I'll submit a change to the docs shortly.

Thanks
Andy

[markstory]

Related pull request is #1638

[markstory]

Changes have been merged, closing.