[infinispan-dev] To Optional or not to Optional?

David M. Lloyd david.lloyd at redhat.com
Thu May 25 11:16:44 EDT 2017


I'm not an Infinispan developer, but I'll chime in anyway. :)

I've never been a fan of Optional.  But the theory behind it that made 
it acceptable in the first place is that it generally gets optimized away.

Now this theory is only true if you never hold a reference to it in any 
persistent manner (it should be very short-lived even as a local 
variable (after optimizations like dead-code & etc. have run)).

What should happen is, the actual allocation should get deleted, and the 
(usually one or two) strictly monomorphic or bi-morphic call(s) should 
each be flattened into what amounts to (at most) simple if/else 
statement(s), all of which should be very fast (as fast as a null-check, 
in theory) and branch-predictable and all that good stuff.  (Now null 
checks have a very slight advantage in that they can be optimistically 
removed in some cases, and only re-added once the operating system gets 
a SIGSEGV or equivalent, but that difference is usually going to be 
pretty small even in fairly tightly optimized code).  This seems 
consistent with the benchmark results.  I doubt it has much to do with 
how hot the code path is (in fact, a hotter code path should mean 
Optional usages will get more accurately optimized by C2 over time). 
With an appropriate JDK build, you can browse the compiler output to 
test this theory out.

If you put an Optional into a field, all of this is very likely to be 
thrown in the garbage.  I think there is some escape analysis stuff that 
might apply but in the most likely case, the heap will simply be 
polluted with a bunch useless crap, along with all the consequences thereof.

So if (and only if) you're using Optional as a return value, creating it 
on the fly (particularly in monomorphic methods), and using the result 
directly via the chainable methods on the Optional class or keeping it 
around only for one or two usages in a local variable (not referring to 
it afterwards in any way), it *should* be fine from a performance 
perspective.  Note that HotSpot is pretty good at knowing the difference 
between when *you* think the value is being referred to and when the 
value is *really* being referred to (this is what caused the finalize() 
debacle that resulted in Runtime.reachabilityFence() being added to Java 
9 - HotSpot was a little *too* good at it).

Aesthetically it's a different story.  There's no magic silver bullet to 
make null problems go away; Optional is a tradeoff just like everything 
in engineering and in life.  I would never put Optional into one of my 
APIs as it exists today (or even as it exists in Java 9, where several 
deficiencies have admittedly been addressed).  I would only start using 
it once it is extremely well-understood and well-established (i.e. maybe 
in 5-10 years I'll have another look).

On 05/25/2017 02:56 AM, Sebastian Laskawiec wrote:
> Indeed Bela, you're an extreme naysayer! :)
> 
> I'm actually trying to get as many comments and arguments out of this 
> discussion. I hope we will be able to iron out a general recommendation 
> or approach how we want to treat Optionals.
> 
> On Tue, May 23, 2017 at 10:14 PM Bela Ban <belaban at mailbox.org 
> <mailto:belaban at mailbox.org>> wrote:
> 
>     Actually, I'm an extreme naysayer! I actually voiced concerns so I'm
>     wondering where your assumption there are no naysayers is coming
>     from... :-)
> 
> 
>     On 23/05/17 1:54 PM, Sebastian Laskawiec wrote:
>      > Hey!
>      >
>      > So I think we have no extreme naysayers to Optional. So let me try to
>      > sum up what we have achieved so:
>      >
>      >   * In macroscale benchmark based on REST interface using Optionals
>      >     didn't lower the performance.
>      >   * +1 for using it in public APIs, especially for those using
>      >     functional style.
>      >   * Creating lots of Optional instances might add some pressure
>     on GC,
>      >     so we need to be careful when using them in hot code paths. In
>      >     such cases it is required to run a micro scale benchamark to make
>      >     sure the performance didn't drop. The microbenchmark should also
>      >     be followed by macro scale benchamrk - PerfJobAck. Also, keep an
>      >     eye on Eden space in such cases.
>      >
>      > If you agree with me, and there are no hard evidence that using
>      > Optional degrade performance significantly, I would like to issue a
>      > pull request and put those findings into contributing guide [1].
>      >
>      > Thanks,
>      > Sebastian
>      >
>      > [1]
>      >
>     https://github.com/infinispan/infinispan/tree/master/documentation/src/main/asciidoc/contributing
>      >
>      > On Mon, May 22, 2017 at 6:36 PM Galder Zamarreño
>     <galder at redhat.com <mailto:galder at redhat.com>
>      > <mailto:galder at redhat.com <mailto:galder at redhat.com>>> wrote:
>      >
>      >     I think Sanne's right here, any differences in such large scale
>      >     test are hard to decipher.
>      >
>      >     Also, as mentioned in a previous email, my view on its usage is
>      >     same as Sanne's:
>      >
>      >     * Definitely in APIs/SPIs.
>      >     * Be gentle with it internals.
>      >
>      >     Cheers,
>      >     --
>      >     Galder Zamarreño
>      >     Infinispan, Red Hat
>      >
>      >     > On 18 May 2017, at 14:35, Sanne Grinovero
>     <sanne at infinispan.org <mailto:sanne at infinispan.org>
>      >     <mailto:sanne at infinispan.org <mailto:sanne at infinispan.org>>>
>     wrote:
>      >     >
>      >     > Hi Sebastian,
>      >     >
>      >     > sorry but I think you've been wasting time, I hope it was
>     fun :)
>      >     This is not the right methodology to "settle" the matter (unless
>      >     you want Radim's eyes to get bloody..).
>      >     >
>      >     > Any change in such a complex system will only affect the
>      >     performance metrics if you're actually addressing the dominant
>      >     bottleneck. In some cases it might be CPU, like if your system is
>      >     at 90%+ CPU then it's likely that reviewing the code to use less
>      >     CPU would be beneficial; but even that can be counter-productive,
>      >     for example if you're having contention caused by optimistic
>      >     locking and you fail to address that while making something else
>      >     "faster" the performance loss on the optimistic lock might become
>      >     asymptotic.
>      >     >
>      >     > A good reason to avoid excessive usage of Optional (and
>      >     *excessive* doesn't mean a couple dozen in a millions lines of
>      >     code..) is to not run out of eden space, especially for all the
>      >     code running in interpreted mode.
>      >     >
>      >     > In your case you've been benchmarking a hugely complex beast,
>      >     not least over REST! When running the REST Server I doubt that
>      >     allocation in eden is your main problem. You just happened to
>     have
>      >     a couple Optionals on your path; sure performance changed but
>      >     there's no enough data in this way to figure out what exactly
>      >     happened:
>      >     >  - did it change at all or was it just because of a lucky
>      >     optimisation? (The JIT will always optimise stuff differently
>     even
>      >     when re-running the same code)
>      >     >  - did the overall picture improve because this code became
>     much
>      >     *less* slower?
>      >     >
>      >     > The real complexity in benchmarking is to accurately understand
>      >     why it changed; this should also tell you why it didn't change
>      >     more, or less..
>      >     >
>      >     > To be fair I actually agree that it's very likely that C2 can
>      >     make any performance penalty disappear.. that's totally possible,
>      >     although it's unlikely to be faster than just reading the field
>      >     (assuming we don't need to do branching because of
>     null-checks but
>      >     C2 can optimise that as well).
>      >     > Still this requires the code to be optimised by JIT first,
>     so it
>      >     won't prevent us from creating a gazillion of instances if we
>      >     abuse its usage irresponsibly. Fighting internal NPEs is a matter
>      >     of writing better code; I'm not against some "Optional" being
>      >     strategically placed but I believe it's much nicer for most
>      >     internal code to just avoid null, use "final", and initialize
>      >     things aggressively.
>      >     >
>      >     > Sure use Optional where it makes sense, probably most on APIs
>      >     and SPIs, but please don't go overboard with it in internals.
>      >     That's all I said in the original debate.
>      >     >
>      >     > In case you want to benchmark the impact of Optional make a JMH
>      >     based microbenchmark - that's interesting to see what C2 is
>      >     capable of - but even so that's not going to tell you much on the
>      >     impact it would have to patch thousands of code all around
>      >     Infinispan. And it will need some peer review before it can tell
>      >     you anything at all ;)
>      >     >
>      >     > It's actually a very challenging topic, as we produce libraries
>      >     meant for "anyone to use" and don't get to set the hardware
>      >     specification requirements it's hard to predict if we should
>      >     optimise the system for this/that resource consumption. Some
>      >     people will have plenty of CPU and have problems with us needing
>      >     too much memory, some others will have the opposite.. the real
>      >     challenge is in making internals "elastic" to such factors and
>      >     adaptable without making it too hard to tune.
>      >     >
>      >     > Thanks,
>      >     > Sanne
>      >     >
>      >     >
>      >     >
>      >     > On 18 May 2017 at 12:30, Sebastian Laskawiec
>      >     <slaskawi at redhat.com <mailto:slaskawi at redhat.com>
>     <mailto:slaskawi at redhat.com <mailto:slaskawi at redhat.com>>> wrote:
>      >     > Hey!
>      >     >
>      >     > In our past we had a couple of discussions about whether we
>      >     should or should not use Optionals [1][2]. The main argument
>      >     against it was performance.
>      >     >
>      >     > On one hand we risk additional object allocation (the Optional
>      >     itself) and wrong inlining decisions taken by C2 compiler [3]. On
>      >     the other hand we all probably "feel" that both of those things
>      >     shouldn't be a problem and should be optimized by C2. Another
>      >     argument was the Optional's doesn't give us anything but as I
>      >     checked, we introduced nearly 80 NullPointerException bugs in two
>      >     years [4]. So we might consider Optional as a way of fighting
>      >     those things. The final argument that I've seen was about lack of
>      >     higher order functions which is simply not true since we have
>      >     #map, #filter and #flatmap functions. You can do pretty amazing
>      >     things with this.
>      >     >
>      >     > I decided to check the performance when refactoring REST
>      >     interface. I created a PR with Optionals [5], ran performance
>      >     tests, removed all Optionals and reran tests. You will be
>      >     surprised by the results [6]:
>      >     >
>      >     > Test case
>      >     > With Optionals [%]    Without Optionals
>      >     > Run 1 Run 2   Avg     Run 1   Run 2   Avg
>      >     > Non-TX reads 10 threads
>      >     > Throughput    32.54   32.87   32.71   31.74   34.04  32.89
>      >     > Response time -24.12  -24.63  -24.38  -24.37  -25.69 -25.03
>      >     > Non-TX reads 100 threads
>      >     > Throughput    6.48    -12.79  -3.16   -7.06   -6.14  -6.60
>      >     > Response time -6.15   14.93   4.39    7.88    6.49 7.19
>      >     > Non-TX writes 10 threads
>      >     > Throughput    9.21    7.60    8.41    4.66    7.15 5.91
>      >     > Response time -8.92   -7.11   -8.02   -5.29   -6.93  -6.11
>      >     > Non-TX writes 100 threads
>      >     > Throughput    2.53    1.65    2.09    -1.16   4.67 1.76
>      >     > Response time -2.13   -1.79   -1.96   0.91    -4.67  -1.88
>      >     >
>      >     > I also created JMH + Flight Recorder tests and again, the
>      >     results showed no evidence of slow down caused by Optionals [7].
>      >     >
>      >     > Now please take those results with a grain of salt since they
>      >     tend to drift by a factor of +/-5% (sometimes even more). But
>     it's
>      >     very clear the performance results are very similar if not
>     the same.
>      >     >
>      >     > Having those numbers at hand, do we want to have Optionals in
>      >     Infinispan codebase or not? And if not, let's state it very
>      >     clearly (and write it into contributing guide), it's because we
>      >     don't like them. Not because of performance.
>      >     >
>      >     > Thanks,
>      >     > Sebastian
>      >     >
>      >     > [1]
>      >
>     http://lists.jboss.org/pipermail/infinispan-dev/2017-March/017370.html
>      >     > [2]
>      >
>     http://lists.jboss.org/pipermail/infinispan-dev/2016-August/016796.html
>      >     > [3]
>      >
>     http://vanillajava.blogspot.ro/2015/01/java-lambdas-and-low-latency.html
>      >     > [4]
>      >
>     https://issues.jboss.org/issues/?jql=project%20%3D%20ISPN%20AND%20issuetype%20%3D%20Bug%20AND%20text%20%7E%20%22NullPointerException%22%20AND%20created%20%3E%3D%202015-04-27%20AND%20created%20%3C%3D%202017-04-27
>      >     > [5] https://github.com/infinispan/infinispan/pull/5094
>      >     > [6]
>      >
>     https://docs.google.com/a/redhat.com/spreadsheets/d/1oep6Was0FfvHdqBCwpCFIqcPfJZ5-5_YYUqlRtUxEkM/edit?usp=sharing
>      >     > [7]
>      >
>     https://github.com/infinispan/infinispan/pull/5094#issuecomment-296970673
>      >     > --
>      >     > SEBASTIAN ŁASKAWIEC
>      >     > INFINISPAN DEVELOPER
>      >     > Red Hat EMEA
>      >     >
>      >     >
>      >     > _______________________________________________
>      >     > infinispan-dev mailing list
>      >     > infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>
>      >     <mailto:infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>>
>      >     > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>      >     >
>      >     > _______________________________________________
>      >     > infinispan-dev mailing list
>      >     > infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>
>      >     <mailto:infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>>
>      >     > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>      >
>      >
>      >     _______________________________________________
>      >     infinispan-dev mailing list
>      > infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>
>     <mailto:infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>>
>      > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>      >
>      > --
>      >
>      > SEBASTIANŁASKAWIEC
>      >
>      > INFINISPAN DEVELOPER
>      >
>      > Red HatEMEA <https://www.redhat.com/>
>      >
>      > <https://red.ht/sig>
>      >
>      >
>      >
>      > _______________________________________________
>      > infinispan-dev mailing list
>      > infinispan-dev at lists.jboss.org
>     <mailto:infinispan-dev at lists.jboss.org>
>      > https://lists.jboss.org/mailman/listinfo/infinispan-dev
> 
>     --
>     Bela Ban, JGroups lead (http://www.jgroups.org)
> 
>     _______________________________________________
>     infinispan-dev mailing list
>     infinispan-dev at lists.jboss.org <mailto:infinispan-dev at lists.jboss.org>
>     https://lists.jboss.org/mailman/listinfo/infinispan-dev
> 
> -- 
> 
> SEBASTIANŁASKAWIEC
> 
> INFINISPAN DEVELOPER
> 
> Red HatEMEA <https://www.redhat.com/>
> 
> <https://red.ht/sig>
> 
> 
> 
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
> 


-- 
- DML


More information about the infinispan-dev mailing list