JDK 13 RFR of JDK-8220346: Refactor java.lang.Throwable to use Objects.requireNonNull

[Peter Levart]

(forgot to send to the list also, so this one if for the list...)

On 3/9/19 3:12 PM, Martin Buchholz wrote:
> As an old lisper, I would personally love to see syntactic 
> abstraction, but also understand that this is not in the spirit of Java.
>
> Given the lack of syntactic abstraction, writing all of those slightly 
> annoying "if" statements *is* in the spirit of Java - so just do it!
>
> lambdas have so much performance magic!  My brain is still working out 
> a performance model.  But no amount of lambda perf machinery will ever 
> beat that "if" statement!

I disagree.

In this particular case, this is not true. A lambda that captures no 
local or instance state is lazily constructed just once (when the lambda 
expression is 1st evaluated) and then it is used as a "local" constant.

The performance model of this statement:

     Objects.requireNonNullElements(defensiveCopy, i-> "stackTrace[" + i 
+ "]");

Is exactly the same as the performance model of this combo:

     static class Holder {
         static final IntFunction<String> messageSupplier = i-> 
"stackTrace[" + i + "]";
     }

// ...and then in code:

     Objects.requireNonNullElements(defensiveCopy, Holder.messageSupplier);

The code in requireNonNullElements contains the same loop and the same 
"if" as the original loop and if statement, so the performance is the 
same as "that "if" statement". There's even a potential to introduce 
further optimization if such logic is extracted into a method as opposed 
to writing in-line loops with ifs (for example with some bulk comparison 
intrinsified logic) so there is even a potential to "beat" that "if" 
statement...

Regards, Peter

>
> On Sat, Mar 9, 2019 at 1:23 AM Peter Levart <peter.levart at gmail.com 
> <mailto:peter.levart at gmail.com>> wrote:
>
>     Hi,
>
>     On 3/8/19 9:35 PM, Martin Buchholz wrote:
>     > On Fri, Mar 8, 2019 at 3:57 AM Tagir Valeev <amaembo at gmail.com
>     <mailto:amaembo at gmail.com>> wrote:
>     >
>     >> Hello!
>     >>
>     >>> diff -r 274361bd6915
>     src/java.base/share/classes/java/lang/Throwable.java
>     >>> --- a/src/java.base/share/classes/java/lang/Throwable.java   
>     Thu Mar 07
>     >>> 10:22:19 2019 +0100
>     >>> +++ b/src/java.base/share/classes/java/lang/Throwable.java   
>     Fri Mar 08
>     >>> 02:06:42 2019 -0800
>     >>> @@ -874,8 +874,7 @@
>     >>>            // Validate argument
>     >>>            StackTraceElement[] defensiveCopy = stackTrace.clone();
>     >>>            for (int i = 0; i < defensiveCopy.length; i++) {
>     >>> -            if (defensiveCopy[i] == null)
>     >>> -                throw new NullPointerException("stackTrace["
>     + i + "]");
>     >>> + Objects.requireNonNull(defensiveCopy[i], "stackTrace[" + i
>     >>> + "]");
>     >> Won't it produce unnecessary garbage due to string concatenation on
>     >> the common case when all frames are non-null?
>     >>
>     > I share Tagir's doubts.  I avoid this construction in
>     performance sensitive
>     > code.
>     > Java doesn't offer syntactic abstraction (can I haz macros?) so
>     the Java
>     > Way is to write the explicit if.
>     >
>     > Logging frameworks have the same trouble.
>     > https://github.com/google/flogger
>     >
>     > Perhaps hotspot's improved automatic NPE reporting makes the
>     message detail
>     > here obsolete?
>
>     If you are referring to the RFR 8218628 by Lindenmaier Goetz which is
>     being proposed on this list, then I believe it will only pertain to
>     situations when the bytecode instructions perform the de-reference of
>     the null reference. In above case, the NPE is thrown explicitly by
>     throw. Well maybe, if the loop was rewritten into:
>
>     for (int i = 0; i < defensiveCopy.length; i++) {
>        defensiveCopy[i].getClass();
>     }
>
>     ...the automatic message could be created, but I doubt that it
>     would be
>     as informative as the custom message above.
>
>     If this is a common situation (from my experience it is and I always
>     write utility methods for it over and over again), then perhaps a new
>     utility method could be added to Objects, like the following:
>
>          /**
>           * Checks that the specified array contains only not {@code
>     null}
>     elements
>           * and throws a customized {@link NullPointerException} if it
>     doesn't.
>           *
>           * <p>This method allows creation of customized message
>     depending
>     on index
>           * of 1st element found to be null.
>           *
>           * @param array           the array to check for nullity of
>     elements
>           * @param messageSupplier supplier of the detail message to be
>           *                        used in the event that an element
>     of the
>     array is
>           *                        {@code null}
>           * @param <T>             the type of the elements
>           * @return {@code array} if the array and all its elements are
>           * not {@code null}
>           * @throws NullPointerException with no message when {@code
>     array}
>     is {@code null}
>           *                              or else with message returned by
>     {@code messageSupplier}
>           *                              when an element of {@code
>     array} is
>     {@code null}.
>           * @since 13
>           */
>          public static <T> T[] requireNonNullElements(T[] array,
>     IntFunction<String> messageSupplier) {
>              for (int i = 0; i < array.length; i++) {
>                  if (array[i] == null) {
>                      throw new NullPointerException(messageSupplier ==
>     null
>     ? null : messageSupplier.apply(i));
>                  }
>              }
>              return array;
>          }
>
>
>     The above for loop could then be re-written as:
>
>          Objects.requireNonNullElements(defensiveCopy, i->
>     "stackTrace[" +
>     i+ "]");
>
>     The lambda in that case is a constant singleton since it does not
>     capture any surrounding state.
>
>
>     Regards, Peter
>
>