Hi Qi, > For example, SingleInputGate is already creating Optional for every > BufferOrEvent in getNextBufferOrEvent(). How much performance gain would we > get if it’s replaced by null check?
When I was introducing it there, I have micro-benchmarked this change, and there was no visible throughput change in a pure network only micro benchmark (with whole Flink running around it any changes would be even less visible). Piotrek > On 5 Aug 2019, at 15:20, Till Rohrmann <trohrm...@apache.org> wrote: > > I'd be in favour of > > - Optional for method return values if not performance critical > - Optional can be used for internal methods if it makes sense > - No optional fields > > Cheers, > Till > > On Mon, Aug 5, 2019 at 12:07 PM Aljoscha Krettek <aljos...@apache.org> > wrote: > >> Hi, >> >> I’m also in favour of using Optional only for method return values. I >> could be persuaded to allow them for parameters of internal methods but I’m >> sceptical about that. >> >> Aljoscha >> >>> On 2. Aug 2019, at 15:35, Yu Li <car...@gmail.com> wrote: >>> >>> TL; DR: I second Timo that we should use Optional only as method return >>> type for non-performance critical code. >>> >>> From the example given on our AvroFactory [1] I also noticed that >> Jetbrains >>> marks the OptionalUsedAsFieldOrParameterType inspection as a warning. >> It's >>> relatively easy to understand why it's not suggested to use (java.util) >>> Optional as a field since it's not serializable. What made me feel >> curious >>> is that why we shouldn't use it as a parameter type, so I did some >>> investigation and here is what I found: >>> >>> There's a JB blog talking about java8 top tips [2] where we could find >> the >>> advice around Optional, there I found another blog telling about the >>> pragmatic approach of using Optional [3]. Reading further we could see >> the >>> reason why we shouldn't use Optional as parameter type, please allow me >> to >>> quote here: >>> >>> It is often the case that domain objects hang about in memory for a fair >>> while, as processing in the application occurs, making each optional >>> instance rather long-lived (tied to the lifetime of the domain object). >> By >>> contrast, the Optionalinstance returned from the getter is likely to be >>> very short-lived. The caller will call the getter, interpret the result, >>> and then move on. If you know anything about garbage collection you'll >> know >>> that the JVM handles these short-lived objects well. In addition, there >> is >>> more potential for hotspot to remove the costs of the Optional instance >>> when it is short lived. While it is easy to claim this is "premature >>> optimization", as engineers it is our responsibility to know the limits >> and >>> capabilities of the system we work with and to choose carefully the point >>> where it should be stressed. >>> >>> And there's another JB blog about code smell on Null [4], which I'd also >>> suggest to read(smile). >>> >>> [1] >>> >> https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95 >>> [2] https://blog.jetbrains.com/idea/2016/07/java-8-top-tips/ >>> [3] >> https://blog.joda.org/2015/08/java-se-8-optional-pragmatic-approach.html >>> [4] https://blog.jetbrains.com/idea/2017/08/code-smells-null/ >>> >>> Best Regards, >>> Yu >>> >>> >>> On Fri, 2 Aug 2019 at 14:54, JingsongLee <lzljs3620...@aliyun.com >> .invalid> >>> wrote: >>> >>>> Hi, >>>> First, Optional is just a wrapper, just like boxed value. So as long as >>>> it's >>>> not a field level operation, I think it is OK to performance. >>>> I think guava optional has a good summary to the uses. [1] >>>>> As a method return type, as an alternative to returning null to >> indicate >>>> that no value was available >>>>> To distinguish between "unknown" (for example, not present in a map) >>>> and "known to have no value" >>>>> To wrap nullable references for storage in a collection that does not >>>> support >>>> The latter two points seem reasonable, but they have few scenes. >>>> >>>> [1] >>>> >> https://github.com/google/guava/blob/master/guava/src/com/google/common/base/Optional.java >>>> >>>> Best, >>>> Jingsong Lee >>>> >>>> >>>> ------------------------------------------------------------------ >>>> From:Timo Walther <twal...@apache.org> >>>> Send Time:2019年8月2日(星期五) 14:12 >>>> To:dev <dev@flink.apache.org> >>>> Subject:Re: [DISCUSS][CODE STYLE] Usage of Java Optional >>>> >>>> Hi everyone, >>>> >>>> I would vote for using Optional only as method return type for >>>> non-performance critical code. Nothing more. No fields, no method >>>> parameters. Method parameters can be overloaded and internally a class >>>> can work with nulls and @Nullable. Optional is meant for API method >>>> return types and I think we should not abuse it and spam the code with >>>> `@SuppressWarnings("OptionalUsedAsFieldOrParameterType")`. >>>> >>>> Regards, >>>> >>>> Timo >>>> >>>> >>>> >>>> Am 02.08.19 um 11:08 schrieb Biao Liu: >>>>> Hi Jark & Zili, >>>>> >>>>> I thought it means "Optional should not be used for class fields". >>>> However >>>>> now I get a bit confused about the edited version. >>>>> >>>>> Anyway +1 to "Optional should not be used for class fields" >>>>> >>>>> Thanks, >>>>> Biao /'bɪ.aʊ/ >>>>> >>>>> >>>>> >>>>> On Fri, Aug 2, 2019 at 5:00 PM Zili Chen <wander4...@gmail.com> wrote: >>>>> >>>>>> Hi Jark, >>>>>> >>>>>> Follow your opinion, for class field, we can make >>>>>> use of @Nullable/@Nonnull annotation or Flink's >>>>>> SerializableOptional. It would be sufficient. >>>>>> >>>>>> Best, >>>>>> tison. >>>>>> >>>>>> >>>>>> Jark Wu <imj...@gmail.com> 于2019年8月2日周五 下午4:57写道： >>>>>> >>>>>>> Hi Andrey, >>>>>>> >>>>>>> I have some concern on point (3) "even class fields as e.g. optional >>>>>> config >>>>>>> options with implicit default values". >>>>>>> >>>>>>> Regarding to the Oracle's guide (4) "Optional should not be used for >>>>>> class >>>>>>> fields". >>>>>>> And IntelliJ IDEA also report warnings if a class field is Optional, >>>>>>> because Optional is not serializable. >>>>>>> >>>>>>> >>>>>>> Do we allow Optional as class field only if the class is not >>>> serializable >>>>>>> or forbid this totally? >>>>>>> >>>>>>> Thanks, >>>>>>> Jark >>>>>>> >>>>>>> On Fri, 2 Aug 2019 at 16:30, Biao Liu <mmyy1...@gmail.com> wrote: >>>>>>> >>>>>>>> Hi Andrey, >>>>>>>> >>>>>>>> Thanks for working on this. >>>>>>>> >>>>>>>> +1 it's clear and acceptable for me. >>>>>>>> >>>>>>>> To Qi, >>>>>>>> >>>>>>>> IMO the most performance critical codes are "per record" code path. >> We >>>>>>>> should definitely avoid Optional there. For your concern, it's "per >>>>>>> buffer" >>>>>>>> code path which seems to be acceptable with Optional. >>>>>>>> >>>>>>>> Just one more question, is there any other code paths which are also >>>>>>>> critical? I think we'd better note that clearly. >>>>>>>> >>>>>>>> Thanks, >>>>>>>> Biao /'bɪ.aʊ/ >>>>>>>> >>>>>>>> >>>>>>>> >>>>>>>> On Fri, Aug 2, 2019 at 11:08 AM qi luo <luoqi...@gmail.com> wrote: >>>>>>>> >>>>>>>>> Agree that using Optional will improve code robustness. However >> we’re >>>>>>>>> hesitating to use Optional in data intensive operations. >>>>>>>>> >>>>>>>>> For example, SingleInputGate is already creating Optional for every >>>>>>>>> BufferOrEvent in getNextBufferOrEvent(). How much performance gain >>>>>>> would >>>>>>>> we >>>>>>>>> get if it’s replaced by null check? >>>>>>>>> >>>>>>>>> Regards, >>>>>>>>> Qi >>>>>>>>> >>>>>>>>>> On Aug 1, 2019, at 11:00 PM, Andrey Zagrebin < >> and...@ververica.com >>>>>>>>> wrote: >>>>>>>>>> Hi all, >>>>>>>>>> >>>>>>>>>> This is the next follow up discussion about suggestions for the >>>>>>> recent >>>>>>>>>> thread about code style guide in Flink [1]. >>>>>>>>>> >>>>>>>>>> In general, one could argue that any variable, which is nullable, >>>>>> can >>>>>>>> be >>>>>>>>>> replaced by wrapping it with Optional to explicitly show that it >>>>>> can >>>>>>> be >>>>>>>>>> null. Examples are: >>>>>>>>>> >>>>>>>>>> - returned values to force user to check not null >>>>>>>>>> - optional function arguments, e.g. with implicit default values >>>>>>>>>> - even class fields as e.g. optional config options with >> implicit >>>>>>>>>> default values >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> At the same time, we also have @Nullable annotation to express >> this >>>>>>>>>> intention. >>>>>>>>>> >>>>>>>>>> Also, when the class Optional was introduced, Oracle posted a >>>>>>> guideline >>>>>>>>>> about its usage [2]. Basically, it suggests to use it mostly in >>>>>> APIs >>>>>>>> for >>>>>>>>>> returned values to inform and force users to check the returned >>>>>> value >>>>>>>>>> instead of returning null and avoid NullPointerException. >>>>>>>>>> >>>>>>>>>> Wrapping with Optional also comes with the performance overhead. >>>>>>>>>> >>>>>>>>>> Following the Oracle's guide in general, the suggestion is: >>>>>>>>>> >>>>>>>>>> - Avoid using Optional in any performance critical code >>>>>>>>>> - Use Optional only to return nullable values in the API/public >>>>>>>> methods >>>>>>>>>> unless it is performance critical then rather use @Nullable >>>>>>>>>> - Passing an Optional argument to a method can be allowed if it >>>>>> is >>>>>>>>>> within a private helper method and simplifies the code, example >>>>>> is >>>>>>> in >>>>>>>>> [3] >>>>>>>>>> - Optional should not be used for class fields >>>>>>>>>> >>>>>>>>>> >>>>>>>>>> Please, feel free to share you thoughts. >>>>>>>>>> >>>>>>>>>> Best, >>>>>>>>>> Andrey >>>>>>>>>> >>>>>>>>>> [1] >>>>>>>>>> >>>>>> >>>> >> http://mail-archives.apache.org/mod_mbox/flink-dev/201906.mbox/%3ced91df4b-7cab-4547-a430-85bc710fd...@apache.org%3E >>>>>>>>>> [2] >>>>>>>>>> >>>>>> >>>> >> https://www.oracle.com/technetwork/articles/java/java8-optional-2175753.html >>>>>>>>>> [3] >>>>>>>>>> >>>>>> >>>> >> https://github.com/apache/flink/blob/master/flink-formats/flink-avro/src/main/java/org/apache/flink/formats/avro/typeutils/AvroFactory.java#L95 >>>>>>>>> >>>> >>>> >>>> >> >>
