Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung 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> 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 walkClass(Function, ? 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 one additional
> commit since the last revision:
>
> review feedback and javadoc clean up
src/java.base/share/classes/java/lang/StackWalker.java line 55:
> 53: * available but not the {@link StackFrame#getDeclaringClass() Class
> reference}.
> 54: * The {@code Class} reference can be accessed if {@link
> Option#RETAIN_CLASS_REFERENCE}
> 55: * RETAIN_CLASS_REFERENCE} option is set.
Double `}` in the link.
src/java.base/share/classes/java/lang/StackWalker.java line 249:
> 247: *
> 248: * @throws UnsupportedOperationException if the {@code
> StackWalker} is of
> 249: * {@link Kind#CLASS_INFO CLASS_INFO} kind
Other CLASS_INFO methods use the javadoc:
if the StackWalker collects class only information
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306129921
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1306143327
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Fri, 25 Aug 2023 13:35:04 GMT, Dan Heidinga wrote:
>> Mandy Chung has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> review feedback and javadoc clean up
>
> src/hotspot/share/classfile/javaClasses.cpp line 2990:
>
>> 2988: // direct calls for private and/or final non-static methods.
>> 2989: flags |= java_lang_invoke_MemberName::MN_IS_METHOD;
>> 2990: }
>
> Both the is_static and the else block set
> java_lang_invoke_MemberName::MN_IS_METHOD. Do we need to differentiate
> between the two cases or can they be collapsed?
yes they can be collapsed. I will update it.
> src/java.base/share/classes/java/lang/ClassFrameInfo.java line 48:
>
>> 46: }
>> 47: boolean isHidden() {
>> 48: return
>> SharedSecrets.getJavaLangInvokeAccess().isHiddenMember(flags &
>> MEMBER_INFO_FLAGS);
>
> Is it better to cache the JLIA in a static final similar to what
> StackFrameInfo does?
either way is fine. I can add a static field.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305941281
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305942726
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung 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> 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 walkClass(Function, ? 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 one additional
> commit since the last revision:
>
> review feedback and javadoc clean up
src/hotspot/share/classfile/javaClasses.cpp line 2990:
> 2988: // direct calls for private and/or final non-static methods.
> 2989: flags |= java_lang_invoke_MemberName::MN_IS_METHOD;
> 2990: }
Both the is_static and the else block set
java_lang_invoke_MemberName::MN_IS_METHOD. Do we need to differentiate between
the two cases or can they be collapsed?
src/java.base/share/classes/java/lang/ClassFrameInfo.java line 48:
> 46: }
> 47: boolean isHidden() {
> 48: return
> SharedSecrets.getJavaLangInvokeAccess().isHiddenMember(flags &
> MEMBER_INFO_FLAGS);
Is it better to cache the JLIA in a static final similar to what StackFrameInfo
does?
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305670313
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1305670016
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 22:03:53 GMT, Mandy Chung wrote: > I like separating this from `Option` as it specifies what information to be > collected from each frame whereas `Option` controls everything else such as > what frames to be filtered In my mind, `Option` _already_ can control what information is to be collected: `RETAIN_CLASS_REFERENCE` as well as what frames to filter: `SHOW_HIDDEN_FRAMES` `SHOW_REFLECT_FRAMES` - PR Comment: https://git.openjdk.org/jdk/pull/15370#issuecomment-1692537575
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung 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> 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 walkClass(Function, ? 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 one additional
> commit since the last revision:
>
> review feedback and javadoc clean up
src/java.base/share/classes/java/lang/StackWalker.java line 47:
> 45: * from the stack frames by default. If the method information is not
> needed,
> 46: * a {@code StackWalker} collecting {@linkplain Kind#CLASS_INFO class
> only information}
> 47: * can be used instead via the {@link #getInstance(Kind, Option...)}
> method.
The description of `Kind` should come after the basics of what StackWalker does:
* The {@link StackWalker#walk walk} method opens a sequential stream
* of {@link StackFrame StackFrame}s for the current thread and then applies
* the given function to walk the {@code StackFrame} stream.
It should be clarified that `METHOD_INFO` returns method information _as well
as_ class information.
Overall I would still prefer adding an `Option.OMIT_METHOD_INFO` over a new
`Kind` enum.
-
PR Review Comment: https://git.openjdk.org/jdk/pull/15370#discussion_r1304929416
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
On Thu, 24 Aug 2023 18:44:14 GMT, Mandy Chung 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> 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 walkClass(Function, ? 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 one additional > commit since the last revision: > > review feedback and javadoc clean up > StackWalkers would be configured along two different axes (Kind and Options). This is intentional. `Kind` is not the best name but it describes what information a stack walker collects from each stack frame. > It changes the mental model, in that all StackWalkers would now be divided > into two Kinds. I feel like this bleeds the implementation into the API a bit. Although this RFE is motivated by an optimized way to walk the stack, each method has a declaring class. In the current implementation, it has an internal way to collect the live locals and operands on the stack (`LOCALS_AND_OPERANDS`). That fits into the third type to be collected from each frame. If that were supported, it would belong to the new enum type. > The existing Option enum already provides a way to configure which frames are > walked, and what information to include in those frames. I think adding a new > Option value fits better. > > It's true that compatibility dictates that the default behavior be to include > method info, so the new option must omit method info. If the NO_METHOD_INFO > is disliked, perhaps a better name can be found - SKIP_METHOD_INFO or > OMIT_METHOD_INFO? In fact it's not so much constrained by the default behavior. I wasn't completely happy with adding `NO_METHOD_INFO` to the options but could live with it. When I reconsidered, I like separating this from `Option` as it specifies what information to be collected from each frame whereas `Option` controls everything else such as what frames to be filtered for better categorization. I updated the javadoc. Maybe it helps? https://cr.openjdk.org/~mchung/api/java.base/java/lang/StackWalker.html https://cr.openjdk.org/~mchung/jdk22/specdiff/overview-summary.html - PR Comment: https://git.openjdk.org/jdk/pull/15370#issuecomment-1692474762
Re: RFR: 8268829: Provide an optimized way to walk the stack with Class object only [v5]
> 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> 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 walkClass(Function, ? 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, > it only needs a small change in calling `getInstance` method to request class > only info... Mandy Chung has updated the pull request incrementally with one additional commit since the last revision: review feedback and javadoc clean up - Changes: - all: https://git.openjdk.org/jdk/pull/15370/files - new: https://git.openjdk.org/jdk/pull/15370/files/30984253..a3a44575 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=15370&range=04 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=15370&range=03-04 Stats: 52 lines in 2 files changed: 11 ins; 7 del; 34 mod Patch: https://git.openjdk.org/jdk/pull/15370.diff Fetch: git fetch https://git.openjdk.org/jdk.git pull/15370/head:pull/15370 PR: https://git.openjdk.org/jdk/pull/15370
