Proposed API for JEP 259: Stack-Walking API

Tue Nov 3 11:28:53 UTC 2015

Hi Mandy,

This is very nice work.

Comments below, mostly minor stuff.

PlatformLogger.java
(and similar comments for duplicated code in LogRecord.java)
—

 542             static final StackWalker stackWalker;

Use capitals for static variable.

 556             private boolean lookingForLogger = true;
 557             /**

Missing line-feed.

 588         private String getCallerInfo() {
 589             Optional<StackWalker.StackFrame> frame = new CallerFinder().get();
 590             if (frame.isPresent()) {
 591                 StackWalker.StackFrame f = frame.get();
 592                 return f.getClassName() + " " + f.getMethodName();
 593             } else {
 594                 return name;
 595             }
 596         }

Consider using:

  return frame.map(f -> f.getClassName() + " " + f.getMethodName()).orElse(name);

You might want to highlight that CallerFinder is a stateful predicate.

LogRecord.java
—

642 // Skip all frames until we have found the first logger frame.
 643 Optional<StackWalker.StackFrame> frame = new CallerFinder().get();
 644 if (frame.isPresent()) {
 645 StackWalker.StackFrame f = frame.get();
 646 setSourceClassName(f.getClassName());
 647 setSourceMethodName(f.getMethodName());
 648 }
 649 // We haven't found a suitable frame, so just punt. This is
 650 // OK as we are only committed to making a "best effort" here.
 651 }

Consider using:

  frame.ifPresent(f -> {
    setSourceClassName(f.getClassName());
    setSourceMethodName(f.getMethodName());
  });

LiveStackFrame
—

  41     /**
  42      * Return the monitors held by this stack frames. This method returns
  43      * an empty array if no monitor is held by this stack frame.
  44      *
  45      * @return the monitors held by this stack frames
  46      */
  47     public Object[] getMonitors();

s/this stack frames/stack frame

Can the contents of the array be modified?

49 /**
 50 * Gets the local variable array of this stack frame.
 51 *
 52 * A single local variable can hold a value of type boolean, byte, char,
 53 * short, int, float, reference or returnAddress. A pair of local varaibles
 54 * can hold a value of type long or double. In other words,
 55 * a value of type long or type double occupies two consecutive local
 56 * variables. For a value of primitive type, the element in the
 57 * local variable array is an {@link PrimitiveValue} object;
 58 * otherwise, the element is an {@code Object}.
 59 *
 60 * A value of type long or double will be represented by
 61 * a {@code PrimitiveValue} object containing the value in the elements
 62 * at index n and n+1 in the local variable array.
 63 *
 64 * @return the local variable array of this stack frame.
 65 */
 66 public Object[] getLocals();

s/varaibles/variables

(double slots are a PITA!)

StackStream
—

Suggest using something other than Stream in the name is this is really about Spliterator and traversal e.g. perhaps StackTraverser. Same for sub-types of StackStream.

 494     @Override
 495     public void forEachRemaining(Consumer<? super StackFrame> action) {
 496         throw new UnsupportedOperationException("Specialized StackWalker");
 497     }
 498
 499     @Override
 500     public boolean tryAdvance(Consumer<? super StackFrame> action) {
 501         throw new UnsupportedOperationException("Specialized StackWalker");
 502     }

Is this required?

StackWalker
—

  46  * The {@linkplain Stream#spliterator() spliterator} of the stream performs
  47  * the stack frame traversal in an {@linkplain Spliterator#ORDERED ordered} manner.

It’s probably best not to mention Spliterator. Suggest

  the stream reports stack frame elements in order, from the top most frame that represents the execution point at
  which the stack was generated to the bottom most frame...

  64  * <p> <em>Examples</em>

Use an @apiNote

  82 public final class StackWalker {
  83     /**
  84      * A {@code StackFrame} object represent a method invocation returned by
  85      * {@link StackWalker}.

s/object represent/object represents

87 * {@code StackFrame} objects returned by {@link StackWalker} may not implement
 88 * the {@link #getDeclaringClass()} method
 89 * depending on the requested {@linkplain Option stack walking options}.

Rather than "may not implement” suggest something like:

  The getDeclaring class method may be unsupported as determined by the stack walking options of a stack walker.

 118          * @throws UnsupportedOperationException if this {@code StackFrame}
 119          *         does not implement this method

if this StackFrame does not support… as determined by …

 137         public String getFileName();
 138         /**

Missing line-feed.

 151         /**
 152          * Returns true if the method containing the execution point
 153          * represented by this stack frame is a native method.
 154          *
 155          * @return {@code true} if the method containing the execution point
 156          *         represented by this stack frame is a native method.
 157          */
 158         public boolean isNativeMethod();

Consistently use {@code true}

 160         /**
 161          * Gets a {@code StackTraceElement} for this stack frame.
 162          *
 163          * @apiNote This method does not seem to be needed as user could simply
 164          * use StackFrame methods directly.
 165          *
 166          * @return {@code StackTraceElement} for this stack frame.
 167          *
 168          * */
 169         public default StackTraceElement asStackTraceElement() {

I guess that @apiNote is a TODO? Perhaps remove it if you think it is not needed, unless it is useful for compatibility reasons, it might be e.g. consider map(StackFrame::asStackTraceElement)

238 private final List<? extends WalkerOption> options;

Do you require the bound here?

 241     /**
 242      * Gets a {@code StackWalker} instance.
 243      *
 244      * <p> This {@code StackWalker} obtains the method name and the class name
 245      * with all {@linkplain StackWalker.Option#SHOW_HIDDEN_FRAMES hidden frames}
 246      * skipped.
 247      */
 248     public StackWalker() {

s/Gets/Creates

Same for other constructor methods.

 252     /**
 253      * Gets a {@code StackWalker} instance with the given options specifying
 254      * the stack frame information it can access.  If no option is specified, this
 255      * {@code StackWalker} obtains the method name and the class name
 256      * with all {@linkplain StackWalker.Option#SHOW_HIDDEN_FRAMES hidden frames}
 257      * skipped.

If no options are given…

It might be easier to state as if the options were {CLASS_REFERENCE, SHOW_REFLECT_FRAMES}, perhaps it depends if the default is someone under specified i.e. is it guaranteed that the option set will be the complement of the single set SHOW_HIDDEN_FRAMES?

I guess any unrecognized options are ignored?

324 /**
 325 * Applies the given function to the stream of {@code StackFrame}s
 326 * for the current thread, traversing from the top of the stack,
 327 * which is the method calling this method.
 328 *
 329 * To find the first 10 calling frames, first skipping those frames whose
 330 * declaring class belong to package {@code com.foo}:
 331 * <blockquote>
 332 * <pre>{@code List<StackFrame> frames = new StackWalker().walk((s) ->}

} -> {

 333      *     s.dropWhile(f -> f.getClassName().startsWith("com.foo."))
 334      *      .limit(10)
 335      *      .collect(Collectors.toList()));
 336      * </pre></blockquote>
 337      * @apiNote The {@code Stream<StackFrame>} object will be closed when
 338      * this method returns.  When a closed {@code Stream<StackFrame>} object
 339      * is reused, {@code IllegalStateException} will be thrown.

Yay, dropWhile :-)

Use an @apiNote for the example, the original note is i think normative.

Probably need to mention that parallel execution will is effectively disabled and stream pipeline execution will only occur on the current thread.

402     @CallerSensitive
 403     public <T> T walk(Function<Stream<StackFrame>, T> function,
 404                       Function<Integer, Integer> batchSizeMapper) {

Can you use IntUnaryOperator?

 430     /**
 431      * Gets the {@code Class} object of the caller invoking the method
 432      * that calls this {@code getCallerClass} method.
 433      *
 434      * <p>This is equivalent to calling
 435      * <blockquote>
 436      * <pre>{@code walk((s) ->}

} -> {

 437      *     s.map(StackFrame::getDeclaringClass)
 438      *      .skip(2)
 439      *      .findFirst()).orElse(Thread.currentThread().getClass());
 440      * </pre></blockquote>

441 *
 442 * For example, {@code Util::getResourceBundle} loads a resource bundle on behalf
 443 * of the caller. It calls this {@code getCallerClass} method to find the method
 444 * calling {@code Util::getResourceBundle} and use the caller's class loader to
 445 * load the resource bundle. The caller class in this example is the {@code Foo} class.

use @apiNote

BatchSizeMapper
—

As usual i always advice using testng and data providers, up to you.

 105         return frameCount = stream
 106                 .map(f -> {
 107                     if (verbose) System.out.println(f);
 108                     return f;
 109                 })

You can use peek instead.

GetCallerClassTest
—

 105 class Test1 {
 106     static void test() {
 107         GetCallerClassTest.staticGetCallerClass(Test1.class);
 108         GetCallerClassTest.reflectiveGetCallerClass(Test1.class);
 109     }
 110 }
 111
 112 class Test2 {
 113     static void test() {
 114         GetCallerClassTest.staticGetCallerClass(Test2.class);
 115         GetCallerClassTest.reflectiveGetCallerClass(Test2.class);
 116     }
 117 }

Duplicate tests?

Suggest renaming tests because on failure it might easier to understand what failed.

HiddenFrames
—

  64     void walk() {
  65         Arrays.stream(new int[]{0})
  66               .forEach(i -> walker.walk(s -> {
  67                   s.forEach(this::checkFrame);
  68                   return null;
  69               }));

Can you use Stream.of(0).forEach instead.

Ran out of time :-)

Paul.

> On 30 Oct 2015, at 20:04, Mandy Chung <mandy.chung at oracle.com> wrote:
> 
> JEP 259:  http://openjdk.java.net/jeps/259
> 
> Javadoc for the proposed StackWalker API:
>   http://cr.openjdk.java.net/~mchung/jdk9/jep259/api/java/lang/StackWalker.html
> 
> A simple way to walk the stack:
> 
>   StackWalker walker = new StackWalker(StackWalker.Option.CLASS_REFERENCE);
>   walker.walk((s) ->  s.filter(f -> interestingClasses.contains(f.getDeclaringClass())).findFirst());
> 
> The current usage of sun.reflect.Reflection.getCallerClass(int depth) can be replaced with this StackWalker API.
> 
> Any feedback on the proposed API is appreciated.
> 
> Mandy
> 
> P.S. webrev of the current implementation:
>   http://cr.openjdk.java.net/~mchung/jdk9/jep259/webrev.00/
> 
> 
> 
> 
> 
> 
> 