+1 to Dan's opinion On Tue, May 23, 2017 at 3:58 PM, Dan Berindei <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> > 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 <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> >>> 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> >>> 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@lists.jboss.org >>> > 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 >>> >>> >>> _______________________________________________ >>> infinispan-dev mailing list >>> infinispan-dev@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@lists.jboss.org >> 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 > -- Vittorio Rigamonti Senior Software Engineer Red Hat <https://www.redhat.com> Milan, Italy vriga...@redhat.com irc: rigazilla <https://red.ht/sig>
_______________________________________________ infinispan-dev mailing list infinispan-dev@lists.jboss.org https://lists.jboss.org/mailman/listinfo/infinispan-dev
