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