Improve event bus threading

[GoogleCodeExporter]

Currently the equinox EventAdmin uses a single thread for dispatching events 
(commands and updates).

Adding a threadpool to the AbstractBinding to process internalReceiveCommand() 
and internalReceiveUpdate() will improve throughput of commands.

See also this thread:

https://groups.google.com/forum/#!topic/openhab/rlcTJgZAT9U

I'm happy to volunteer for this task ;-)

Original issue reported on code.google.com by davy.van...@gmail.com on 15 Sep 2013 at 11:42

[GoogleCodeExporter]

Original comment by kai.openhab on 15 Sep 2013 at 2:33

• Changed state: Accepted

[GoogleCodeExporter]

I've attached a patch containing an implementation of asynchronous handling of 
posted commands and updates.
With these changes, one of my rules dropped from a total execution time of 7 
seconds to 2 seconds.

Any commands which are published using sendCommand will still block delivery of 
other events until that call has finished.
All non-blocking calls using postCommand or postUpdate are handled in a 
threadpool with a minimum size of 1 and a maximum size of 5.  The maximum size 
can be overriden using 
a system property.

Original comment by davy.van...@gmail.com on 16 Sep 2013 at 9:58

• Changed state: Started

Attachments:

• 454-eventbus.patch

[GoogleCodeExporter]

Original comment by davy.van...@gmail.com on 17 Sep 2013 at 8:21

• Changed state: ReadyForReview
• Added labels: Version-1.4.0
• Removed labels: Version-later

[GoogleCodeExporter]

Jan-Willem, would you be interested to do the review for this? You seem to be 
deeply into this topic as well. We could continue further discussions here, if 
necessary.

Original comment by kai.openhab on 18 Sep 2013 at 2:12

[GoogleCodeExporter]

Dave,

I've removed a few extra line breaks and I've removed the unneccesary 
conversion of the async boolean to string, boxing / unboxing is enough. Patch 
on patch is attached.

Then there is the class EventSubscriberThreadPool that is currently activated 
from CoreActivator. I think that it's nicer to create an OSGi service from this 
class and have OSGi manage it, like the rest of openHAB. Do you feel you're up 
to that?

We could also opt for putting in inside a org.openhab.core.threading package 
and calling it GenericThreadPool so that we have a system wide thread pool that 
everybody can use to execute Runnable tasks. How would we feel about that?

Original comment by jwsp...@gmail.com on 18 Sep 2013 at 5:05

Attachments:

• 454-eventbus.2.patch

[GoogleCodeExporter]

I'll handle this one.

Original comment by jwsp...@gmail.com on 18 Sep 2013 at 5:06

• Changed state: Accepted

[GoogleCodeExporter]

Davy, working with a Dave in my day job I misspelled your name, sorry :-)

Original comment by jwsp...@gmail.com on 18 Sep 2013 at 8:10

[GoogleCodeExporter]

Hi Jan-Willem, no worries about the name.  Can happen to all of us.

I'm not really clear on why you changed the code to an unsafe cast, surely that 
will trigger a Sonar violation. (if openHAB ever gets Sonar, which I really 
hope it does once we are on git)

You're right about an OSGI Service being nicer and making it more generic is a 
also good idea.  I was also already thinking about a more generic service, but 
in stead of a system wide thread pool, I was thinking about a ThreadPoolService 
which can handle multiple pools.

I'm a bit strapped for time at the moment, but I should be able to finish it 
before the end of the week.  Expect a new patch soon...

Original comment by davy.van...@gmail.com on 18 Sep 2013 at 8:22

[GoogleCodeExporter]

OK, when looking at the code again I just remembered why I didn't make it an 
OSGI service in the first place. It would require that the service declarations 
of all bindings would need to be updated to inject the service into the 
binding. I don't see any other options (except for piggybacking on the 
eventpublisher, but that's really ugly), so it's back to the Activator approach 
for now... 

Original comment by davy.van...@gmail.com on 18 Sep 2013 at 9:57

[GoogleCodeExporter]

I think a cast to get a boolean into and from a dictionary of objects (actually 
boxing/unboxing) is still better than converting it into a string and vice 
versa. I'm also not sure why sonar wouldn't trigger a warning on your cast of 
object to string.

Commands and items are also cast upon taking them out of the dictionary, 
nothing wrong with that.

Original comment by jwsp...@gmail.com on 19 Sep 2013 at 10:41

[GoogleCodeExporter]

I probably didn't choose the right words. What I was trying to say is that the 
new cast is not safe for null objects, whilst before null values were not an 
issue.

Anyway, let's do the review on the new patch later this week..

Original comment by davy.van...@gmail.com on 19 Sep 2013 at 11:08

[GoogleCodeExporter]

You're right about the service declarations. But that would be just a search 
replace, including the binding archetype bundle.

I would not like piggybacking it onto the publisher either. E.g. where would 
you get the ThreadPool from if your just a subscriber.

Anyway, I'll wait for the new patch.

Original comment by jwsp...@gmail.com on 19 Sep 2013 at 2:40

[GoogleCodeExporter]

Another small thing: Maybe catch the numberformatexception on

Integer.parseInt(prop)

In case somebody tries to enter a non-numeric config value for the number of 
threads.

Original comment by jwsp...@gmail.com on 19 Sep 2013 at 6:16

[GoogleCodeExporter]

Hi Jan-Willem, a new version is available for review on github:
https://github.com/dvanherbergen/openhab/commit/29a97d3162925070fa4d197bf741f49a
da010f51

Davy

Original comment by davy.van...@gmail.com on 21 Sep 2013 at 7:23

[GoogleCodeExporter]

Thanks Davy,

First impression is really good. I'll try and find some time tomorrow to review 
it, pretty swamped with my day job this week.

Original comment by jwsp...@gmail.com on 24 Sep 2013 at 8:48

[GoogleCodeExporter]

Thanks Jan-Willem.  There's no rush on my side, so whenever you can fit it in 
is fine for me.

Original comment by davy.van...@gmail.com on 24 Sep 2013 at 3:44

[GoogleCodeExporter]

Davy, I think it's ready for inclusion. I've notified kai to have a look at it 
as well and then merge it. Thanks.

Original comment by jwsp...@gmail.com on 1 Oct 2013 at 8:34

[GoogleCodeExporter]

Right, thanks Jan-Willem, it's on my todo list to reply soon - Davy, you might 
want to want to wait for me to provide feedback as well.

Original comment by kai.openhab on 1 Oct 2013 at 9:17

• Changed state: Started

[GoogleCodeExporter]

Kai, Jan-Willem, I've cancelled this changeset for now.  After thinking about 
this and issue 453 some more, I believe the way bindings are handled needs a 
bigger overhaul, especially in the context of the upcoming split between 
Eclipse SmartHome and openHAB.
I've got some ideas on this, but need to do some testing to see how feasible 
they are.  
I'll post my progress in this issue..

Davy

Original comment by davy.van...@gmail.com on 8 Oct 2013 at 9:01

[GoogleCodeExporter]

Hi Davy,
Thanks for the info, it was just in time before I went on to including it :-)
I am fine with reconsidering things with the split of the projects - it might 
indeed have some impacts (or rather opportunities to make more fundamental 
changes).
Btw, The OSGi framework "Concierge" comes with an EventAdmin service, which 
seems to be multi-threaded - as Concierge is on my wishlist as a replacement 
for Equinox once it is available, this might change the situation again as well.

Original comment by kai.openhab on 9 Oct 2013 at 7:30

[GoogleCodeExporter]

Migrated to https://bugs.eclipse.org/bugs/show_bug.cgi?id=423517

Original comment by kai.openhab on 7 Dec 2013 at 10:22

• Changed state: Invalid