I haven't checked Sebastian's refactored code, but does it use Optionals as a *field* type? That's misuse (same as using it as an arg), it's intended solely as method return type.
Radim On 05/23/2017 05:45 PM, Katia Aresti wrote: > 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.berin...@gmail.com > <mailto:dan.berin...@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 > <slask...@redhat.com <mailto:slask...@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 > > <https://github.com/infinispan/infinispan/tree/master/documentation/src/main/asciidoc/contributing> > > On Mon, May 22, 2017 at 6:36 PM Galder Zamarreño > <gal...@redhat.com <mailto:gal...@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 > <sa...@infinispan.org <mailto:sa...@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 > <slask...@redhat.com <mailto:slask...@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 > > <http://lists.jboss.org/pipermail/infinispan-dev/2017-March/017370.html> > > [2] > > http://lists.jboss.org/pipermail/infinispan-dev/2016-August/016796.html > > <http://lists.jboss.org/pipermail/infinispan-dev/2016-August/016796.html> > > [3] > > http://vanillajava.blogspot.ro/2015/01/java-lambdas-and-low-latency.html > > <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 > > <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 > <https://github.com/infinispan/infinispan/pull/5094> > > [6] > > https://docs.google.com/a/redhat.com/spreadsheets/d/1oep6Was0FfvHdqBCwpCFIqcPfJZ5-5_YYUqlRtUxEkM/edit?usp=sharing > > <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 > > <https://github.com/infinispan/infinispan/pull/5094#issuecomment-296970673> > > -- > > SEBASTIAN ŁASKAWIEC > > INFINISPAN DEVELOPER > > Red Hat EMEA > > > > > > _______________________________________________ > > infinispan-dev mailing list > > infinispan-dev@lists.jboss.org > <mailto:infinispan-dev@lists.jboss.org> > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > <https://lists.jboss.org/mailman/listinfo/infinispan-dev> > > > > _______________________________________________ > > infinispan-dev mailing list > > infinispan-dev@lists.jboss.org > <mailto:infinispan-dev@lists.jboss.org> > > https://lists.jboss.org/mailman/listinfo/infinispan-dev > <https://lists.jboss.org/mailman/listinfo/infinispan-dev> > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > <mailto:infinispan-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/infinispan-dev > <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@lists.jboss.org > <mailto:infinispan-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/infinispan-dev > <https://lists.jboss.org/mailman/listinfo/infinispan-dev> > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org <mailto:infinispan-dev@lists.jboss.org> > https://lists.jboss.org/mailman/listinfo/infinispan-dev > <https://lists.jboss.org/mailman/listinfo/infinispan-dev> > > > > > _______________________________________________ > infinispan-dev mailing list > infinispan-dev@lists.jboss.org > https://lists.jboss.org/mailman/listinfo/infinispan-dev -- Radim Vansa <rva...@redhat.com> JBoss Performance Team _______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
