Proposed API for JEP 259: Stack-Walking API

Mandy Chung mandy.chung at oracle.com
Wed Nov 4 03:50:23 UTC 2015


> On Nov 3, 2015, at 3:28 AM, Paul Sandoz <paul.sandoz at oracle.com> wrote:
> 
> Hi Mandy,
> 
> This is very nice work.
> 
> Comments below, mostly minor stuff.
> 

Thanks for the review.

I fixed most of the comments below.   One question:

Is the name “StackStream" inappropriate and its subtypes?  I prefer StackStream to StackTraverser.

> 
> PlatformLogger.java
> (and similar comments for duplicated code in LogRecord.java)
>> 
> 542             static final StackWalker stackWalker;
> 
> Use capitals for static variable.
> 

Fixed.

> 
> 556             private boolean lookingForLogger = true;
> 557             /**
> 
> Missing line-feed.
> 

Fixed. 
> 
> 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);
> 

Thanks for the nicer alternative.

> 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());
>  });
> 


Fixed.

> 
> 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?
> 
> 

Yes this can be modified.  Each stack frame has its own arrays.

>  49     /**
>  50      * Gets the local variable array of this stack frame.
>  51      *
>  52      * <p>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      * <p>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 <em>n</em> and <em>n</em>+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!)
> 

Yes it is.

> 
> 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?
> 

CallerClassStream and StackTrace don’t support to use with Stream API for performance.
> 
> 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      * <p> {@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?
> 

Sorry WalkerOption has been removed.  I only updated the API but not webrev.00.

> 
> 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?

In fact the default can’t be expressed with the current options.  This is a good point though and I have been considering adding SHOW_METHOD_INFO which will be the default.   I’ll update the API.

> 
> I guess any unrecognized options are ignored?
> 

Now it takes only Option enums.

> 
> 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      * <p>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?
> 

Remi and David suggest that too.

> 
> 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      * <p>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.

I started this one with TestNG but this data provider requires the method to return Object[][].  I automatically resorted to old-school test :)

> 
> 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?
> 

Probably extra-caution and can be removed.

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

Right - I have to review the tests one more pass before posting the updated webrev.

> 
> 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.
> 

Yes Stream.of is better.


> 
> Ran out of time :-)

Thanks a lot.

Mandy




More information about the core-libs-dev mailing list