RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v4]

[Daniel Fuchs]

On Wed, 23 Aug 2023 20:27:38 GMT, Mandy Chung <mchung at openjdk.org> wrote:

>> 8268829: Provide an optimized way to walk the stack with Class object only
>> 
>> `StackWalker::walk` creates one `StackFrame` per frame and the current implementation
>> allocates one `StackFrameInfo` and one `MemberName` objects per frame. Some frameworks
>> like logging may only interest in the Class object but not the method name nor the BCI,
>> for example, filters out its implementation classes to find the caller class.  It's
>> similar to `StackWalker::getCallerClass` but allows a predicate to filter out the element.
>> 
>> This PR proposes to add `StackWalker.Kind` enum to specify the information that a stack walker
>> collects.  If no method information is needed, a `StackWalker` of `CLASS_INFO` can be used
>> instead and such stack walker will save the overhead (1) to extract the method information
>> and (2) the memory used for the stack walking.   In addition, this can also fix
>> 
>> - [8311500](https://bugs.openjdk.org/browse/JDK-8311500): StackWalker.getCallerClass() throws UOE if invoked reflectively
>> 
>> New factory methods to take a parameter to specify the kind of stack walker to be created are defined.
>> This provides a simple way for existing code, for example logging frameworks, to take advantage of
>> this enhancement with the least change as it can keep the existing function for traversing
>> `StackFrame`s.
>> 
>> For example: to find the first caller filtering a known list of implementation class,
>> existing code can call `StackWalker::getInstance(CLASS_INFO, ...)` to create a stack walker instance:
>> 
>> 
>>      StackWalker walker = StackWalker.getInstance(Kind.CLASS_INFO, Option.RETAIN_CLASS_REFERENCE);
>>      Optional<Class<?>> callerClass = walker.walk(s ->
>>              s.map(StackFrame::getDeclaringClass)
>>               .filter(interestingClasses::contains)
>>               .findFirst());
>> 
>> 
>> If method information is accessed on the `StackFrame`s produced by this stack walker such as
>> `StackFrame::getMethodName`, then `UnsupportedOperationException` will be thrown.
>> 
>> #### Alternatives Considered
>> One alternative is to provide a new API:
>> `<T> T walkClass(Function<? super Stream<Class<?>, ? extends T> function)`
>> 
>> In this case, the caller would need to pass a function that takes a stream
>> of `Class` object instead of `StackFrame`.  Existing code would have to
>> modify calls to the `walk` method to `walkClass` and the function body.
>> 
>> Another alternative is to add a new `NO_METHOD_INFO` option.  Similar to the proposed API,
>>...
>
> Mandy Chung has updated the pull request incrementally with three additional commits since the last revision:
> 
>  - fix whitespace
>  - move retainClassRef to ClassFrameInfo as a bit set in the flags field
>  - fixup the factory methods

src/java.base/share/classes/java/lang/ClassFrameInfo.java line 32:

> 30: class ClassFrameInfo implements StackFrame {
> 31:     protected Object classOrMemberName;    // Class or ResolvedMemberName initialized by VM
> 32:     protected int flags;

I see only one place where flags is written to and that's at line 35 below. I guess the reason it's not final is that it can be mutated by the VM. Maybe that would deserve a comment here:
 ```suggestion
     protected int flags; // flags can be mutated by the VM to indicate hidden frame

src/java.base/share/classes/java/lang/StackStreamFactory.java line 1083:

> 1081:     private static boolean filterStackWalkImpl(Class<?> c) {
> 1082:         return stackWalkImplClasses.contains(c) ||
> 1083:                 c.getPackageName().equals("java.util.stream");

There is a small semantic difference here and in the change below compared to the original code: the original code would have also included sub-packages, where the new code will not. Since neither `java.util.stream` nor `java.lang.invoke` have sub-package I guess it's of no concern for now.

src/java.base/share/classes/java/lang/StackStreamFactory.java line 1096:

> 1094:                c == Constructor.class ||
> 1095:                MethodAccessor.class.isAssignableFrom(c) ||
> 1096:                ConstructorAccessor.class.isAssignableFrom(c);

I guess LambdaForm have the hidden flag on, which is why there's no need to include them here now?

src/java.base/share/classes/java/lang/StackWalker.java line 333:

> 331:          * {@linkplain StackFrame#getMethodType() method type},
> 332:          * {@linkplain StackFrame#getLineNumber() line number} and
> 333:          * {@linkplain StackFrame#getByteCodeIndex() bytecode index}.

Maybe you should include `getFileName` (and possibly `isNativeMethod`) here?

src/java.base/share/classes/java/lang/StackWalker.java line 338:

> 336:     }
> 337: 
> 338:     enum ExtendedOption {

Should `ExtendedOption` now be renamed into `ExtendedKind`?

test/jdk/java/lang/StackWalker/SanityTest.java line 120:

> 118:                 sw.forEach(StackWalker.StackFrame::isNativeMethod));
> 119:         assertThrows(UnsupportedOperationException.class, () ->
> 120:                 sw.forEach(StackWalker.StackFrame::toStackTraceElement));

Should we check that the exception is thrown by each frame? This code will hit the exception at the first frame and not check the others.

-------------

PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304261309
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304358574
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304362430
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304380080
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304424800
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304440971