Skip to content

Conversation

@mlchung
Copy link
Member

@mlchung mlchung commented Aug 21, 2023

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
StackFrames.

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 StackFrames 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 option creates ClassFrameInfo[]
buffer that is filled by the VM during stack walking. StackFrameInfo is updated to be
a subclass of ClassFrameInfo and stores ResolvedMethodName and all other method information.

ClassFrameInfo holds the Class instance plus flags which indicate if it's hidden and caller-sensitive.
Hence StackWalker::getCallerClass can take advantage of this change to fix
JDK-8311500: StackWalker.getCallerClass() throws UOE if invoked reflectively. StackWalker::getCallerClasswill createClassFrameInfo[]` buffer instead and do the check in Java instead of in the VM.

Performance

The microbenchmark shows that the runtime performance of stack walking with method information
is 15-31% faster than the old implementation. A StackWalker with DROP_METHOD_INFO
is about 21-43% faster compared to the StackWalker collecting the method information.

The memory usage of the data structure (shown by -XX:PrintFieldLayout):

Type Instance size
ClassFrameInfo 24 bytes
StackFrameInfo 48 bytes
ResolvedMethodName 24 bytes
MemberName 48 bytes
StackFrameInfo (old) 32 bytes

The existing implementation allocates a total of 104 bytes for each frame
(StackFrameInfo + MemberName + ResolvedMethodName). MemberName is designed
to represent methods and fields and include additional fields that stack walker does not needed.
So StackFrameInfo is modified to store ResolvedMethodName directly and include additional
fields to keep the name and type of the method that will be expanded when such info is requested.
The new implementation allocates a total of 72 bytes for each frame (StackFrameInfo + ResolvedMethodName)
which saves 30% of the buffer memory when collecting method information.
For a stack walker collecting just class information, 24 bytes is allocated for each frame
as only ClassFrameInfo is needed. In addition, it saves the overhead in creating ResolvedMethodName
object in the VM.

StackWalker::getCallerClass

The new implementation of StackWalker::getCallerClass is about ~3% degradation.
The old implementation creates a Class array of size 8 whereas the new implementation
which creates a ClassFrameInfo array of size 8 initialized with 6 ClassFrameInfo elements.
The new implementation of getCallerClass adds 144 bytes more, which is insignificant.
The benefit of this is to do the filtering in Java rather than doing in VM.


Progress

  • Change must be properly reviewed (1 review required, with at least 1 Reviewer)
  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change requires CSR request JDK-8314811 to be approved

Issues

  • JDK-8268829: Provide an optimized way to walk the stack with Class object only (Enhancement - P4)
  • JDK-8210375: StackWalker::getCallerClass throws UnsupportedOperationException (Bug - P3)
  • JDK-8314811: Provide an optimized way to walk the stack with Class object only (CSR)

Reviewers

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.org/jdk.git pull/15370/head:pull/15370
$ git checkout pull/15370

Update a local copy of the PR:
$ git checkout pull/15370
$ git pull https://git.openjdk.org/jdk.git pull/15370/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 15370

View PR using the GUI difftool:
$ git pr show -t 15370

Using diff file

Download this PR as a diff file:
https://git.openjdk.org/jdk/pull/15370.diff

Webrev

Link to Webrev Comment

@bridgekeeper
Copy link

bridgekeeper bot commented Aug 21, 2023

👋 Welcome back mchung! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

@openjdk
Copy link

openjdk bot commented Aug 21, 2023

@mlchung The following labels will be automatically applied to this pull request:

  • build
  • core-libs
  • hotspot
  • security

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing lists. If you would like to change these labels, use the /label pull request command.

@openjdk openjdk bot added security security-dev@openjdk.org hotspot hotspot-dev@openjdk.org build build-dev@openjdk.org core-libs core-libs-dev@openjdk.org labels Aug 21, 2023
@mlchung
Copy link
Member Author

mlchung commented Aug 21, 2023

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@mlchung
Copy link
Member Author

mlchung commented Aug 21, 2023

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@openjdk openjdk bot removed the build build-dev@openjdk.org label Aug 21, 2023
@openjdk
Copy link

openjdk bot commented Aug 21, 2023

@mlchung
The build label was successfully removed.

@openjdk openjdk bot removed the security security-dev@openjdk.org label Aug 21, 2023
@openjdk
Copy link

openjdk bot commented Aug 21, 2023

@mlchung
The security label was successfully removed.

@mlchung mlchung changed the title Stackwalker class 8268829: Provide a lighter weighted API to walk the stack with Class object only Aug 21, 2023
@openjdk openjdk bot added the rfr Pull request is ready for review label Aug 21, 2023
@mlchung mlchung changed the title 8268829: Provide a lighter weighted API to walk the stack with Class object only 8268829: Provide an optimized way to walk the stack with Class object only Aug 21, 2023
@mlchung
Copy link
Member Author

mlchung commented Aug 21, 2023

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@mlchung
Copy link
Member Author

mlchung commented Aug 30, 2023

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@AlanBateman
Copy link
Contributor

Indeed, Set.of factory methods are easy to use. I'm okay with taking it out.

It would be easy to add in the future if needed, say if more options were introduced and the common case requires 2 or more options.

@AlanBateman
Copy link
Contributor

The API changes in the the current update (111661b) look good.

Copy link
Member

@bchristi-git bchristi-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@openjdk
Copy link

openjdk bot commented Sep 6, 2023

@mlchung This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8268829: Provide an optimized way to walk the stack with Class object only
8210375: StackWalker::getCallerClass throws UnsupportedOperationException

Reviewed-by: coleenp, dfuchs, bchristi

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 84 new commits pushed to the master branch:

  • 0c865a7: 8315637: JDK-8314249 broke libgraal
  • 683672c: 8292692: Move MethodCounters inline functions out of method.hpp
  • 9bf3dee: 8314831: NMT tests ignore vm flags
  • b74805d: 8315863: [GHA] Update checkout action to use v4
  • 1cae0f5: 8315220: Event NativeLibraryLoad breaks invariant by taking a stacktrace when thread is in state _thread_in_native
  • 8f7e29b: 8313422: test/langtools/tools/javac 144 test classes uses com.sun.tools.classfile library
  • 8557205: 8312569: RISC-V: Missing intrinsics for Math.ceil, floor, rint
  • 2fd870a: 8315444: Convert test/jdk/tools to Classfile API
  • 81f8c57: 8314632: Intra-case dominance check fails in the presence of a guard
  • b408a82: 8314260: Unable to load system libraries on Windows when using a SecurityManager
  • ... and 74 more: https://git.openjdk.org/jdk/compare/c8acab1d913a6c676706fce7ad98a7f831a95682...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

@openjdk openjdk bot added ready Pull request is ready to be integrated and removed csr Pull request needs approved CSR before integration labels Sep 6, 2023
Copy link
Member

@bchristi-git bchristi-git left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks great - thanks!

static StackWalker walker(String name) {
return switch (name) {
case "default" -> WALKER;
case "class+method" -> WALKER;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

don't you need to also change "default" into "class+method" in the @Param({"default", "class_only"}) annotations below?

Copy link
Member Author

@mlchung mlchung Sep 7, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@mlchung
Copy link
Member Author

mlchung commented Sep 7, 2023

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

@openjdk
Copy link

openjdk bot commented Sep 7, 2023

Going to push as commit 111ecdb.
Since your change was applied there have been 87 commits pushed to the master branch:

  • 716201c: 8314935: Shenandoah: Unable to throw OOME on back-to-back Full GCs
  • 4c6d7fc: 8315795: runtime/Safepoint/TestAbortVMOnSafepointTimeout.java fails after JDK-8305507
  • 7e7ab6e: 8315877: ProblemList vmTestbase/nsk/jvmti/InterruptThread/intrpthrd003/TestDescription.java on macosx-aarch64
  • 0c865a7: 8315637: JDK-8314249 broke libgraal
  • 683672c: 8292692: Move MethodCounters inline functions out of method.hpp
  • 9bf3dee: 8314831: NMT tests ignore vm flags
  • b74805d: 8315863: [GHA] Update checkout action to use v4
  • 1cae0f5: 8315220: Event NativeLibraryLoad breaks invariant by taking a stacktrace when thread is in state _thread_in_native
  • 8f7e29b: 8313422: test/langtools/tools/javac 144 test classes uses com.sun.tools.classfile library
  • 8557205: 8312569: RISC-V: Missing intrinsics for Math.ceil, floor, rint
  • ... and 77 more: https://git.openjdk.org/jdk/compare/c8acab1d913a6c676706fce7ad98a7f831a95682...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot added the integrated Pull request has been integrated label Sep 7, 2023
@openjdk openjdk bot closed this Sep 7, 2023
@openjdk openjdk bot removed ready Pull request is ready to be integrated rfr Pull request is ready for review labels Sep 7, 2023
@openjdk
Copy link

openjdk bot commented Sep 7, 2023

@mlchung Pushed as commit 111ecdb.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.

// type is not a MethodType yet. Convert it thread-safely.
synchronized (this) {
if (type instanceof String sig) {
type = JLIA.getMethodType(sig, declaringClass().getClassLoader());

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe there should be a return here:

Suggested change
type = JLIA.getMethodType(sig, declaringClass().getClassLoader());
return type = JLIA.getMethodType(sig, declaringClass().getClassLoader());

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

type is of type Object, don't think this compiles as the result type of = is the type variable's type.

Copy link
Member Author

@mlchung mlchung Sep 18, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @mlchung, thanks for making a comment in an OpenJDK project!

All comments and discussions in the OpenJDK Community must be made available under the OpenJDK Terms of Use. If you already are an OpenJDK Author, Committer or Reviewer, please click here to open a new issue so that we can record that fact. Please Use "Add GitHub user mlchung" for the summary.

If you are not an OpenJDK Author, Committer or Reviewer, simply check the box below to accept the OpenJDK Terms of Use for your comments.

Your comment will be automatically restored once you have accepted the OpenJDK Terms of Use.

*
* @see StackWalker.Option#DROP_METHOD_INFO
*/
class ClassFrameInfo implements StackFrame {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This class can be sealed:

Suggested change
class ClassFrameInfo implements StackFrame {
sealed class ClassFrameInfo implements StackFrame permits StackFrameInfo {

@wenshao
Copy link
Contributor

wenshao commented May 2, 2025

The value of flags can only be 0 or 0x08000000, so flags & MEMBER_INFO_FLAGS in isHidden and isCallerSensitive methods are always false

    ClassFrameInfo(StackWalker walker) {
        this.flags = walker.retainClassRef ? RETAIN_CLASS_REF_BIT : 0;
    }

    private static final int MEMBER_INFO_FLAGS    = 0x00FFFFFF;
    private static final int RETAIN_CLASS_REF_BIT = 0x08000000; // retainClassRef

The flags field is only assigned in the ClassFrameInfo constructor, so flags can be set to final, and the value here will only be 0 or RETAIN_CLASS_REF_BIT (0x08000000).

So flags & MEMBER_INFO_FLAGS in the following code is always false

    boolean isCallerSensitive() {
        return JLIA.isCallerSensitive(flags & MEMBER_INFO_FLAGS);
    }

    boolean isHidden() {
        return JLIA.isHiddenMember(flags & MEMBER_INFO_FLAGS);
    }

@wenshao
Copy link
Contributor

wenshao commented May 2, 2025

Sorry, I misread it. There is a comment in flags: updated by VM to set hidden and caller-sensitive bits

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

core-libs core-libs-dev@openjdk.org hotspot hotspot-dev@openjdk.org integrated Pull request has been integrated

Development

Successfully merging this pull request may close these issues.