[infinispan-dev] To Optional or not to Optional?
Katia Aresti
karesti at redhat.com
Tue May 23 11:45:50 EDT 2017
Dan, I disagree with point 2 where you say "You now have a field that could
be null, Optional.empty(), or Optional.of(something)"
This is the point of optional. You shouldn't have a field that has these 3
possible values, just two of them = Some or None. If the field is mutable,
it should be initialised to Optional.empty(). In the case of an API,
Optional implicitly says that the return value can be empty, but when you
return a "normal" object, either the user reads the doc, either will have
bugs or boilerplate code defending from the possible null value (even if
never ever this API will return null)
:o)
Cheers
On Tue, May 23, 2017 at 3:58 PM, Dan Berindei <dan.berindei at gmail.com>
wrote:
> I wouldn't say I'm an extreme naysayer, but I do have 2 issues with
> Optional:
>
> 1. Performance becomes harder to quantify: the allocations may or may not
> be eliminated, and a change in one part of the code may change how
> allocations are eliminated in a completely different part of the code.
> 2. My personal opinion is it's just ugly... instead of having one field
> that could be null or non-null, you now have a field that could be null,
> Optional.empty(), or Optional.of(something).
>
> Cheers
> Dan
>
>
>
> On Tue, May 23, 2017 at 1:54 PM, Sebastian Laskawiec <slaskawi at redhat.com>
> 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>
>> 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>
>>> 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>
>>> 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/1oep6Was
>>> 0FfvHdqBCwpCFIqcPfJZ5-5_YYUqlRtUxEkM/edit?usp=sharing
>>> > [7] https://github.com/infinispan/infinispan/pull/5094#issuecomm
>>> ent-296970673
>>> > --
>>> > SEBASTIAN ŁASKAWIEC
>>> > INFINISPAN DEVELOPER
>>> > Red Hat EMEA
>>> >
>>> >
>>> > _______________________________________________
>>> > infinispan-dev mailing list
>>> > infinispan-dev at lists.jboss.org
>>> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>> >
>>> > _______________________________________________
>>> > infinispan-dev mailing list
>>> > infinispan-dev at lists.jboss.org
>>> > https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>>
>>>
>>> _______________________________________________
>>> infinispan-dev mailing list
>>> infinispan-dev at lists.jboss.org
>>> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>>
>> --
>>
>> SEBASTIAN ŁASKAWIEC
>>
>> INFINISPAN DEVELOPER
>>
>> Red Hat EMEA <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
>>
>
>
> _______________________________________________
> infinispan-dev mailing list
> infinispan-dev at lists.jboss.org
> https://lists.jboss.org/mailman/listinfo/infinispan-dev
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: http://lists.jboss.org/pipermail/infinispan-dev/attachments/20170523/27da71a1/attachment-0001.html
More information about the infinispan-dev
mailing list