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

[Brent Christian]

On Thu, 31 Aug 2023 17:09:40 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 `Option::DROP_METHOD_INFO` enum that requests to drop the method information.  If no method information is needed, a `StackWalker` with `DROP_METHOD_INFO`
>> can be used instead and such stack walker will save the overhead of extracting the method information
>> and the memory used for the stack walking.
>> 
>> 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 create a stack walker instance with `DROP_METHOD_INFO` option:
>> 
>> 
>>      StackWalker walker = StackWalker.getInstance(Option.DROP_METHOD_INFO, Option.RETAIN_CLASS_REFERENCE);
>>      Optional<Class<?>> callerClass = walker.walk(s ->
>>              s.map(StackFrame::getDeclaringClass)
>>               .filter(Predicate.not(implClasses::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.
>> 
>> #### Javadoc & specdiff
>> 
>> https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html
>> https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html
>> 
>> #### 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.
>> 
>> ### Implementation Details
>> 
>> A `StackWalker` configured with `DROP_METHOD_INFO` ...
>
> Mandy Chung has updated the pull request with a new target base due to a merge or a rebase. The pull request now contains 29 commits:
> 
>  - Merge
>  - Remove the new getInstance method taking varargs
>  - update mode to be int rather than long
>  - update tests
>  - Review feedback on javadoc
>  - Revised the API change.  Add Option::DROP_METHOD_INFO
>  - Review feedback from Remi
>  - fixup javadoc
>  - Review feedback: move JLIA to ClassFrameInfo
>  - review feedback and javadoc clean up
>  - ... and 19 more: https://git.openjdk.org/jdk/compare/c8acab1d...111661bc

Looks good. I like `DROP_METHOD_INFO`. I have just a few minor comments.

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

> 655:     static final class ClassFrameBuffer extends FrameBuffer<ClassFrameInfo> {
> 656:         final StackWalker walker;
> 657:         ClassFrameInfo[] classFrames;      // caller class for fast path

Maybe I missed it, but I don't see any differences between `ClassFramesBuffer` and `StackFrameBuffer` other than the `ClassFrameInfo`/`StackFrameInfo` types. Could a single, generified Buffer class serve for both?

test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 64:

> 62:             default -> throw new IllegalArgumentException(name);
> 63:         };
> 64:     }

The previous `WALKER_DEFAULT` would not have retained the Class reference, but the new `default` will?

test/micro/org/openjdk/bench/java/lang/StackWalkBench.java line 360:

> 358:          }
> 359:      }
> 360: 

I'm fine having this benchmark active by default, though offline we had discussed adding it commented out.

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

PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1603548954
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1312375997
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1316143011
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1316205105