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

[Volker Simonis]

On Fri, 25 Aug 2023 22:22:43 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.
>> 
>> #### 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 hav...
>
> Mandy Chung has updated the pull request incrementally with two additional commits since the last revision:
> 
>  - fixup javadoc
>  - Review feedback: move JLIA to ClassFrameInfo

Hi Mandy,

thanks for doing this experiment. I've looked at your proposal, but I can't see how it can help to fix [JDK-8311500](https://bugs.openjdk.org/browse/JDK-8311500). Your change proposes to add a bunch of new Java SE specific changes which can't be downported to any previous JDK version but [JDK-8311500](https://bugs.openjdk.org/browse/JDK-8311500) affects JDK 11, 17 and 21. I'd therefore still appreciate if we could fix [JDK-8311500](https://bugs.openjdk.org/browse/JDK-8311500) first before we start to redesign the public StackWalker API.

In case you do redesign the `StackWalker` API, I fully agree with @bchristi-git that the current version is overkill. As your [specdiff](https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html) shows, it introduces 34 API changes (9 additions and 18 changes) with no clear benefit. I second @bchristi-git that an addtional `StackWalker.Option` would be simpler and much cleaner (I'd vote for `SKIP_METHOD_INFO`) if we decide to change the API at all..

Another point that should be taken into account when we start to redesign the StackWalker API is its interaction with the `-XX:+ShowHiddenFrames` command line parameter. This is currently not specified and can lead to surprising results (see e.g. [this remark about failing JTreg tests with `-XX:+ShowHiddenFrames`](https://github.com/openjdk/jdk/pull/14773#issuecomment-1631065562)).

I'm also not convinced that doing frame filtering in Java is the right thing to do. If we have to call into the JVM anyway, I think it would be more natural to let the JVM do as much of the work as possible. To me it seems as if your change is mostly about improving the performance of stack walks and I think this could be achieved to an even greater extent without the need to change the public API, simply by improving the implementation. Such improvements could than also be downported to JDK 17 & 21 which isn't possible with your approach.

If it turns out that API changes are required in order to achieve superior performance, we should try to keep them to a minimum and weight them against the performance benefits. E.g. we could add a single `getCallerClass(int depth)` method which would provide all the benefits of `Kind.CLASS_INFO` but with considerably less API changes.

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

PR Review: https://git.openjdk.org/jdk/pull/15370#pullrequestreview-1598457114