spring-attic / reactive-streams-commons

A joint research effort for building highly optimized Reactive-Streams compliant operators.
Apache License 2.0
357 stars 51 forks source link

Use shaded dependency on JCTools instead of copy and paste #18

Open nitsanw opened 8 years ago

nitsanw commented 8 years ago

Currently this library uses some queue implementations lifted in spirit and to some extent code out of JCTools. The 2 projects share a contributor, @akarnokd. I would recommend this project directly shades JCTools and uses the originals. Versioned dependencies allow for clear release boundaries and well understood state of code while also enabling future improvements and corrections to be fed in easily. This is why everyone does it...

smaldini commented 8 years ago

For the context we talk about https://github.com/reactor/reactive-streams-commons/blob/master/src/main/java/reactivestreams/commons/util/SpscArrayQueue.java and https://github.com/reactor/reactive-streams-commons/blob/master/src/main/java/reactivestreams/commons/util/SpscLinkedArrayQueue.java

I need to evaluate what else JCTools offer nowadays, @akarnokd any thought ?

akarnokd commented 8 years ago

Rsc is a research effort and is thought as a basis for other reactive libraries which can have their own notion of scheduling and queueing. Therefore, Rsc was designed to be parametrizable by the queue implementation through a Supplier in each relevant operator. We did this because Reactor was mainly using Disruptor as its queue-like backing data structure. Thus, if one chooses to drive Rsc with JCTools queues, he/she is free to do so.

I didn't use JCTools directly because:

nitsanw commented 8 years ago

The choice is yours. Let me see if I understand the argument:

I understand JCTools may not be a good fit if you want to avoid the paddind etc. But the AtomicSpsc is notionally targeting this exact usecase. Why not collaborate to provide a common solution? This hits on another obejction to C&P dependencies, they make a bad relationship in that they block the consuming library authors from improving the origin.

akarnokd commented 8 years ago

I thought you weren't concerned with queue performance, just wanted a known quantity

With our Spsc, it's more about allocation than queue performance at the moment. SpscAtomicArrayQueue allocates 1 parent class, 2 AtomicLong, 1 AtomicReferenceArray and 1 backing array. By inlining the field updaters, Rsc's Spsc allocates 1 parent class and 1 array. Unfortunately, the linked-array spsc can only save on the 2 AtomicLong instances compared to JCTools' version. This allocation amount is dominant for short-lived reactive sequences.

Why not collaborate to provide a common solution?

I don't think you could encode all the configuration option in a single class: to pad index fields or not, pad elements or not, use lookahead or not, support exact capacity or not. I consider both Rsc and JCTools as a template where one can extract and customize them to his/her liking.

I'll allocate some time and do a comparative benchmark of the queues in our dataflow context.

nitsanw commented 8 years ago

AFU change can be implemented for JCTools, might be worth it. I'm similarly open to extending the AtomicRefArray (you objected last we discussed it, but I actually have no issue with it. If people down cast from Queue or use the other methods directly they are shooting their own foot.)

"..I don't think you could encode all the configuration option in a single class.." This is something I mean to address using bytecode generation.

akarnokd commented 8 years ago

I've run a comparison benchmark and got interesting results.

image

with errors:

image

I expected unsafe to be better, but I'm surprised how the growable and unbounded variants do even better in some cases.

Also, Rsc's spsc queue has around 5-30% more overhead; could be the index trashing or the field updater overhead.

image

akarnokd commented 8 years ago

It seems the main source of lost throughput was the index trashing:

image

The rsc column is the original plain SpscArrayQueue, the rsc1 had just item padding and the rsc2 has index padding but no item padding.

Comparing against the unsafe version:

image