search

bennidi / mbassador

Powerful event-bus optimized for high throughput in multi-threaded applications. Features: Sync and Async event publication, weak/strong references, event filtering, annotation driven
MIT License
958 stars 146 forks source link

Concurrent map #99

Closed dorkbox closed 9 years ago

dorkbox commented 9 years ago

starting place for enhancements. This by itself isn't a huge improvement, as ultimately the async threading model is what needs to be tweaked. ConcurrentHashMapV8 really shines once those tweaks are made.

Changing the IConcurrentSet -> Set, simplifies testing with different data structures, since there are many different Weak/Strong/Soft concurrent implementations available.

dorkbox commented 9 years ago

out of curiosity, what's the logic behind having both a dispatch (2 threads) and invoker (N threads). Wouldn't it be just as effective to go directly to the invoker, and not have to hand off objects as often?

I'm running into issues back porting some of my concurrent modification to the subscription manager, since they rely on just having invoker threads (the extra dispatch threads break it).

dorkbox commented 9 years ago

Let me revisit if ConcurrentHashMaps are exactly necessary, since it might not be. At one point they were needed, as I tried to make subscribe/unsubscribe fully concurrent - which turns out to really complicate everything, so I didn't pursue it further.

bennidi commented 9 years ago

Yeah, I was trying to do the same but did not find a model to decouple writes from reads. It is mainly due to the case when two adjacent listeners are removed concurrently. Removal operations cut out their target by joining its neighbours. Different interleavings for the scenario of adjacent removals could end up leaving one of the two listeners still in the list (one operation might overwrite the other).

I think it is really tricky to fully parallelize reads and writes. I don't feel able to handle this complexity - ingenuity did not strike me yet :)

I think it is not worth it because the code is quite clean and not overly complex. And it works. And it's still quite fast.

2015-02-25 17:27 GMT+01:00 dorkbox notifications@github.com:

Let me revisit if ConcurrentHashMaps are exactly necessary, since it might not be. At one point they were needed, as I tried to make subscribe/unsubscribe fully concurrent - which turns out to really complicate everything, so I didn't pursue it further.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-75992464.

dorkbox commented 9 years ago

One idea, found while looking at different collections, is the "AtomicReference" object, which might do the trick, since a compare-and-swap could be used. Maybe? It's a deep hole to go down, for sure.

bennidi commented 9 years ago

It might be worth to consider in the future. But I would capture this idea in another issue and move on. I think there are other optimizations that are easier to achieve.

2015-02-25 18:50 GMT+01:00 dorkbox notifications@github.com:

One idea, found while looking at different collections, is the "AtomicReference" object, which might do the trick, since a compare-and-swap could be used. Maybe? It's a deep hole to go down, for sure.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76016358.

dorkbox commented 9 years ago

yeah.

What are you thoughts on removing the dispatch thread model, and having async calls added directly to the queue? It simplifies things quite a lot, and it would make porting changes much easier.

On Wed, Feb 25, 2015 at 7:58 PM, Benjamin Diedrichsen < notifications@github.com> wrote:

It might be worth to consider in the future. But I would capture this idea in another issue and move on. I think there are other optimizations that are easier to achieve.

2015-02-25 18:50 GMT+01:00 dorkbox notifications@github.com:

One idea, found while looking at different collections, is the "AtomicReference" object, which might do the trick, since a compare-and-swap could be used. Maybe? It's a deep hole to go down, for sure.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76016358.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76030562.

bennidi commented 9 years ago

I think I do not yet understand the implications of such a change. There ARE these two different notions of synchronicity. Merging them into a single queue would be quite a change to the API and the internal processing model. It has influence on processing order. I agree that the current model makes the code more complex, for sure.

I think such a change can not easily be made without getting some feedback from the user base. Can you create an issue and describe the change and its implications such that we can have people give feedback?

I do not mean to complicate things but since we are not the only users I think it should be discussed by more people.

2015-02-25 20:14 GMT+01:00 dorkbox notifications@github.com:

yeah.

What are you thoughts on removing the dispatch thread model, and having async calls added directly to the queue? It simplifies things quite a lot, and it would make porting changes much easier.

On Wed, Feb 25, 2015 at 7:58 PM, Benjamin Diedrichsen < notifications@github.com> wrote:

It might be worth to consider in the future. But I would capture this idea in another issue and move on. I think there are other optimizations that are easier to achieve.

2015-02-25 18:50 GMT+01:00 dorkbox notifications@github.com:

One idea, found while looking at different collections, is the "AtomicReference" object, which might do the trick, since a compare-and-swap could be used. Maybe? It's a deep hole to go down, for sure.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76016358.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76030562.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76033909.

dorkbox commented 9 years ago

Hm, so on a synchronous publish, it still goes to the invoker queue? (I thought the invoker queue only was for async publish)

On Wed, Feb 25, 2015 at 8:47 PM, Benjamin Diedrichsen < notifications@github.com> wrote:

I think I do not yet understand the implications of such a change. There ARE these two different notions of synchronicity. Merging them into a single queue would be quite a change to the API and the internal processing model. It has influence on processing order. I agree that the current model makes the code more complex, for sure.

I think such a change can not easily be made without getting some feedback from the user base. Can you create an issue and describe the change and its implications such that we can have people give feedback?

I do not mean to complicate things but since we are not the only users I think it should be discussed by more people.

2015-02-25 20:14 GMT+01:00 dorkbox notifications@github.com:

yeah.

What are you thoughts on removing the dispatch thread model, and having async calls added directly to the queue? It simplifies things quite a lot, and it would make porting changes much easier.

On Wed, Feb 25, 2015 at 7:58 PM, Benjamin Diedrichsen < notifications@github.com> wrote:

It might be worth to consider in the future. But I would capture this idea in another issue and move on. I think there are other optimizations that are easier to achieve.

2015-02-25 18:50 GMT+01:00 dorkbox notifications@github.com:

One idea, found while looking at different collections, is the "AtomicReference" object, which might do the trick, since a compare-and-swap could be used. Maybe? It's a deep hole to go down, for sure.

— Reply to this email directly or view it on GitHub <https://github.com/bennidi/mbassador/pull/99#issuecomment-76016358 .

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76030562.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76033909.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76040877.

bennidi commented 9 years ago

So, there is the queue and a set of dispatcher threads taking care of async publish. Then there is the executor service that takes care of async handler invocation. Those are the two different aspects of synchronicity. And the configuration of both is exposed via Feature class. So they are part of the public interface which makes it harder to change.

dorkbox commented 9 years ago

Ah, ok.

On Fri, Feb 27, 2015 at 11:15 AM, Benjamin Diedrichsen < notifications@github.com> wrote:

So, there is the queue and a set of dispatcher threads taking care of async publish. Then there is the executor service that takes care of async handler invocation. Those are the two different aspects of synchronicity. And the configuration of both is exposed via Feature class. So they are part of the public interface which makes it harder to change.

— Reply to this email directly or view it on GitHub https://github.com/bennidi/mbassador/pull/99#issuecomment-76369767.

dorkbox commented 9 years ago

I just removed the concurrentmap stuff + some other small polish

dorkbox commented 9 years ago

RE: Synchronicity - I think you're right about changing that -- it would impact others usage in ways that are undetermined. I think it's best to leave that alone.

bennidi commented 9 years ago

Hehe...too late. I already merged the PR :) I thought I will give it a try and go through the code in my IDE. I think that the concurrent map could actually help in the subscription manager (which still uses locks). Will report back on that matter. You can put the polish on top of master in another PR. I would also suggest that you cherry pick the commits onto an up-to-date master branch so that no merge commit is necessary.

dorkbox commented 9 years ago

sure.