On Wed, 23 Aug 2023 20:27:38 GMT, Mandy Chung <mch...@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
