Repository navigation
JCTools as Shaded Dependency #1735
Description
Now that JCTools is available on Maven Central (http://search.maven.org/#search%7Cga%7C1%7Cjctools) we should consider depending on it directly instead of copy/pasting as we have done so far: https://github.com/ReactiveX/RxJava/tree/1.x/src/main/java/rx/internal/util/unsafe (note the README that provides background and details).
There are a few considerations:
-
RxJava is a "zero dependency" project so the dependency will need to be shaded (package names changed to be inside RxJava) so that it becomes part of the RxJava jar.
-
Byte size of pulling in code from JCTools that we don't need. This affects Android particularly.
-
Use of JCTools must all be done behind a conditional check
UnsafeAccess.isUnsafeAvailable()otherwise RxJava will not work on platforms withoutsun.misc.Unsafe.
Activity
benjchristensen commentedon Oct 8, 2014
Here is an example of doing this from Couchbase: https://github.com/couchbase/couchbase-jvm-core/blob/master/build.gradle#L166
benjchristensen commentedon Oct 8, 2014
/cc @nitsanw
daschl commentedon Oct 8, 2014
@benjchristensen as it turned out, no complaints yet on shading it.
The only thing I found so far is that since we are not shipping the sources for the shaded jars, it's hard to debug and load sources when you try to debug. So I need to figure out how to re-package the shaded sources and ship them with the sources jar so debugging is easy as well.
If you know how, I'd be happy to apply it.
akarnokd commentedon Jan 26, 2015
JCTools has different philosophy about when a queue is full. For example, trying to put 128 elements into an empty queue in response to a request can yield
MissingBackpressureExceptionbecauseoffer()returns false.nitsanw commentedon Jan 28, 2015
@akarnokd I'm not sure which queue you are describing or what version... can you be more specific? Is this an issue in the copied over code? in v1.0? in trunk?
akarnokd commentedon Jan 28, 2015
@nitsanw There are so many versions flying around and my knowledge might be out of date or my perception misaligned. In PR #2541, the code I've copied meets our requirement, but I'm a bit cautious because what happened with SpscGrowableArrayQueue's capacity debate.
nitsanw commentedon Jan 28, 2015
@akarnokd part of the joy of development, SpscGrowableArrayQueue was prematurely released, and your help in debugging is appreciated. As discussed on that topic, the Queue interface does not support a capacity() method and the provided capacity in constructors is a open to some interpretation. General attitude in JCTools is that the capacity will be the larger or equal to the capacity prescribed, usually rounded to the nearest power of 2.
akarnokd commentedon Jan 28, 2015
Okay, given a power of 2 initial capacity of the queues, RxJava expects them to hold that amount of values exactly if there are no consumers.
nitsanw commentedon Jan 28, 2015
I wonder if having a capacity() method which lets you know what the actual capacity it would be a help?
I'm planning to release new Queue interfaces with the next version of JCTools.
akarnokd commentedon Jan 28, 2015
You misunderstand me. Any JCTools queue that we use has to pass the following test.
nitsanw commentedon Jan 28, 2015
I understand, but I'm not sure I agree :-)
benjchristensen commentedon Jan 28, 2015
Just because our app needs this use case does not mean the underlying queue needs to support it outside the power of 2.
@akarnokd We can address this just by making it so our sizes are always a power of 2. This is why I've always stuck to a power of 2 for our sizing so that we don't have to layer an extra check on top and can just rely on
offerto reject.20 remaining items
akarnokd commentedon Jun 17, 2016
A PR is welcome setting this up, however, there are 3 problems that need addressing:
guidomedina commentedon Jun 18, 2016
This was successfully done at Netty:
I would send a PR, but my Gradle knowledge is at most basic to get to the point of using a plugin, it was difficult enough in Maven because we had to also make the dependency optional for OSGI and available for tests on the parent pom.
For single producer/consumer, I would use the atomic version because one thread won't race against anything else hence no making a performance difference against
Unsafe; the real difference when usingUnsafewill come when using either multiple producer or consumer or both.nitsanw commentedon Jun 18, 2016
@akarnokd I know the Gradle shade plugin is slightly flakier than the maven one, so that might prove a blocker here. You should be able to make this work using explicit includes/excludes.
The double arg SPSC is a usecase I plan to support via code generated channels, for now you might have to extend the class somehow or keep your variant.
@guidomedina The difference between Unsafe/Atomic performance is down to indirection costs. I think your intuition on this is incorrect. There's no difference in contention in principal, in fact using atomics may reduce contention because having slower consumer/producer code means the contended memory is hit less frequently.
guidomedina commentedon Jun 18, 2016
@nitsanw there are more ops to be made that I agree, but in practice you get a 100% hit rate on get, add or both hence making the difference in cost to tend to zero, I agree that my previous statement technically speaking was wrong but the practical difference is what?
That in the scenario of only one thread at head and tail concurrently.
nitsanw commentedon Jun 18, 2016
@guidomedina please reflect your intended usecase in a JMH benchmark, let the numbers speak ;-)
guidomedina commentedon Jun 18, 2016
Ratio using Java8u92 i5-4670k after
Unsafehas warmed up is 0.4 indicating for a single thread it is twice faster, but is that the same for Android?nitsanw commentedon Jun 18, 2016
Post your benchmark as gist please.
Why do you think Unsafe is 2 times faster? Use -prof perfnorm and reason on counters?
guidomedina commentedon Jun 18, 2016
https://gist.github.com/guidomedina/d20381b55c89c641ccdb5e277edcd1b8
nitsanw commentedon Jul 22, 2016
Note that Netty now properly depends on JCTools. Also note recent improvements to SPSC linked array queues (not implemented in the atomic cases).
akarnokd commentedon Jul 22, 2016
@nitsanw
I saw you unified a lot of implementations. I'd kept them separate so no excess but unused features sneak
into the hot path. In addition, there are now extra fields and local variables which taxes the JIT and may not leave room for local variables outside the offer/poll.
Plus it still doesn't have two object offer which saves us a tuple allocation / spinning on a poll.
Btw, I'd be interested in a blog post about how the JIT's register allocator behaves when there are more locals than registers. I had odd and lower benchmark results in some drain loops: when I cached a final field, that was slower than reading the field in a loop, surrounded by unrelated volatile reads.
nitsanw commentedon Jul 22, 2016
@akarnokd I'm happy to discuss, but not all of your comment seems focused on this thread
"Plus it still doesn't have two object offer which saves us a tuple allocation / spinning on a poll." -> true
Unrelated:
"I saw you unified a lot of implementations. I'd kept them separate so no excess but unused features sneak into the hot path. " -> This is all in the offerColdPath. This is as bad as a bimorphic call, so no worse than inheritance, just uglier. There's 2 unused fields in the Unbounded case. Because there's no clean way to extend a class with layout control (I want extra fields in the producer fields clamp), this may not be easily avoidable. Can maybe reorder the inheritance order and add them in extension.
The implementation for Growable is much much simpler, and has a lot in common with the chunked, I think that worked out well. Overall code size has reduced. I've re-benchmarked and the results look good...
As for number of var/fields impact on register allocation, while in theory larger methods can confuse the register allocator in principal the number of registers on a typical CPU is quite high and running out of registers is not that common. The case you describe should be examined in isolation. I'd be happy to have a look.
nitsanw commentedon Jul 23, 2016
@akarnokd I played around with the impls some more. Reordered fields and specialized cold path. Perf looks good. Please give feedback in JCTools.
akarnokd commentedon Oct 18, 2017
Closing as this will likely not happen for 1.x. Subsequent versions have to use customized versions anyway.