Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

8271514: support JFR use of new ThreadsList::Iterator #4949

Closed
wants to merge 14 commits into from

Conversation

dcubed-ojdk
Copy link
Member

@dcubed-ojdk dcubed-ojdk commented Jul 30, 2021

A trivial fix to support JFR use of new ThreadsList::Iterator.

This fix was tested with Mach5 Tier[1-3].


Progress

  • Change must not contain extraneous whitespace
  • Commit message must refer to an issue
  • Change must be properly reviewed

Issue

  • JDK-8271514: support JFR use of new ThreadsList::Iterator

Reviewers

Contributors

  • Kim Barrett <kbarrett@openjdk.org>

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4949/head:pull/4949
$ git checkout pull/4949

Update a local copy of the PR:
$ git checkout pull/4949
$ git pull https://git.openjdk.java.net/jdk pull/4949/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4949

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4949.diff

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 30, 2021

👋 Welcome back dcubed! 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 openjdk bot commented Jul 30, 2021

@dcubed-ojdk The following label will be automatically applied to this pull request:

  • hotspot

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

@openjdk openjdk bot added the hotspot label Jul 30, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2021

⚠️ @dcubed-ojdk This pull request contains merges that bring in commits not present in the target repository. Since this is not a "merge style" pull request, these changes will be squashed when this pull request in integrated. If this is your intention, then please ignore this message. If you want to preserve the commit structure, you must change the title of this pull request to Merge <project>:<branch> where <project> is the name of another project in the OpenJDK organization (for example Merge jdk:master).

@dcubed-ojdk dcubed-ojdk changed the base branch from master to pr/4671 Jul 30, 2021
@dcubed-ojdk dcubed-ojdk changed the base branch from pr/4671 to pr/4948 Jul 30, 2021
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Jul 31, 2021

/label remove hotspot

/label add hotspot-runtime
/label add serviceability

@openjdk openjdk bot removed the hotspot label Jul 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 31, 2021

@dcubed-ojdk
The hotspot label was successfully removed.

@openjdk openjdk bot added the hotspot-runtime label Jul 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 31, 2021

@dcubed-ojdk
The hotspot-runtime label was successfully added.

@openjdk openjdk bot added the serviceability label Jul 31, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 31, 2021

@dcubed-ojdk
The serviceability label was successfully added.

@dcubed-ojdk dcubed-ojdk marked this pull request as ready for review Jul 31, 2021
@openjdk openjdk bot added the rfr label Jul 31, 2021
@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 31, 2021

Webrevs

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Jul 31, 2021

/contributor add @kimbarrett

@openjdk
Copy link

@openjdk openjdk bot commented Jul 31, 2021

@dcubed-ojdk
Contributor Kim Barrett <kbarrett@openjdk.org> successfully added.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Dan,

I'm assuming that the intent of this conversion is that JavaThreadIteratorWithHandle is going to be removed - is that right?

The conversion seems okay but definitely not a trivial change IMO. And a query below before I can approve this.

Thanks,
David

@@ -47,15 +47,17 @@ class JfrThreadIterator : public AP {

class JfrJavaThreadIteratorAdapter {
private:
JavaThreadIteratorWithHandle _iter;
JavaThread* _next;
ThreadsListHandle _tlist;
Copy link
Member

@dholmes-ora dholmes-ora Aug 2, 2021

Choose a reason for hiding this comment

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

Why do we need to store this?

It looks very suspiocious to have a member that is a stackObj, in a class that is not itself a stackObj. ??

Copy link
Contributor

@sspitsyn sspitsyn Aug 4, 2021

Choose a reason for hiding this comment

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

The _tlist is used locally in JfrJavaThreadIteratorAdapter constructor only, so it is possible to get rid of it for the price of complicating the constructor a little bit.

Choose a reason for hiding this comment

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

The _tlist is a handle that ensures the captured ThreadsList remains live while the iterator (or a copy) exists. Dropping it would leave _it and _end potentially dangling.

Copy link
Member

@dholmes-ora dholmes-ora Aug 5, 2021

Choose a reason for hiding this comment

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

Okay I had assumed there was a ThreadsList/Handle in the enclosing scope that was being used to initialize this and keep things live, but that is not the case.
I still think it makes no sense to have a StackObj subtype as a member though. Maybe ThreadsListHandle should no longer be a StackObj ?

Choose a reason for hiding this comment

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

This isn't a change wrto StackObj usage. This adapter class used to have a member that was a JavaThreadIteratorWithHandle (the WithHandle class for the same reason the new code has a ThreadsListHandle, to keep the associated ThreadsList alive), which also derives from StackObj.

Personally, I want to agree with you. I would (and do) use StackObj far more sparingly than it appears in our code base. But I've had this same argument with other folks. The de facto situation, so far as I understand it, is that deriving from StackObj documents normal usage. It has no operational behavior or additional semantics, other than attempting to prevent heap allocation (and it doesn't really succeed there, since a derived class could override that behavior, though I don't currently know of any that do so). That seems to be what some people want from it and think is useful, and I've stopped trying to convince them otherwise.

Copy link

@mgronlun mgronlun Aug 5, 2021

Choose a reason for hiding this comment

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

The JfrThreadIterator is only used as a stack-allocated object, so it and the adapters can be decorated with StackObj to better communicate this intent. The AP (Allocation Policy) policy in JfrThreadIterator can be removed and the JfrThreadIterator can be decorated with StackObj (unconditionally). Depends on how much you want to spend on this (I would guess minimal). But in actual usages, the iterators and adapters are de-facto StackObj already.

Copy link
Member

@dholmes-ora dholmes-ora Aug 5, 2021

Choose a reason for hiding this comment

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

Ah I didn't realise JTIWH was also stackObj.

The thing with StackObj to me is that it indicates a local object that does something interesting on construction and then also on destruction (eg. like MutexLocker). So having one as a member where the destruction is not related to a stack scope seems odd to me. But a stackObj member in a stackObj class seems more reasonable.

Copy link
Contributor

@coleenp coleenp Aug 6, 2021

Choose a reason for hiding this comment

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

StackObj to me communicates intent. I don't have to look for "new SomeClass" to see how it is allocated and deallocated, since the new and delete operator are private and won't compile. Same is true for AllStatic. I'm one that hasn't been convinced otherwise. I'm not totally intransigent though as i approved the removal of ValueObj. So there's that.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Dan,
It looks good to me modulo the question from David.
Thanks,
Serguei

@kimbarrett
Copy link

@kimbarrett kimbarrett commented Aug 5, 2021

I'm assuming that the intent of this conversion is that JavaThreadIteratorWithHandle is going to be removed - is that right?

That was my intent. But this is a somewhat complicated use, so dealing with it separately. And I agree this is not a trivial change.

@openjdk-notifier openjdk-notifier bot changed the base branch from pr/4948 to master Aug 6, 2021
@openjdk-notifier
Copy link

@openjdk-notifier openjdk-notifier bot commented Aug 6, 2021

The dependent pull request has now been integrated, and the target branch of this pull request has been updated. This means that changes from the dependent pull request can start to show up as belonging to this pull request, which may be confusing for reviewers. To remedy this situation, simply merge the latest changes from the new target branch into this pull request by running commands similar to these in the local repository for your personal fork:

git checkout JDK-8271513
git fetch https://git.openjdk.java.net/jdk master
git merge FETCH_HEAD
# if there are conflicts, follow the instructions given by git merge
git commit -m "Merge master"
git push

@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

@dcubed-ojdk 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:

8271514: support JFR use of new ThreadsList::Iterator

Co-authored-by: Kim Barrett <kbarrett@openjdk.org>
Reviewed-by: sspitsyn, mgronlun

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 9 new commits pushed to the master branch:

  • d04d4ee: 8274894: Use Optional.empty() instead of ofNullable(null) in HttpResponse.BodySubscribers.discarding
  • 33050f8: 8274986: max code printed in hs-err logs should be configurable
  • 8de2636: 8274615: Support relaxed atomic add for linux-aarch64
  • 7d2633f: 8275002: Remove unused AbstractStringBuilder.MAX_ARRAY_SIZE
  • cfe7471: 8177814: jdk/editpad is not in jdk TEST.groups
  • a5f09d1: 8275031: runtime/ErrorHandling/MachCodeFramesInErrorFile.java fails when hsdis is present
  • ef0922e: 8274560: JFR: Add test for OldObjectSample event when using Shenandoah
  • 1e30695: 8274466: G1: use field directly rather than method in G1CollectorState::in_mixed_phase
  • dd93c6e: 8272167: AbsPathsInImage.java should skip *.dSYM directories

Please see this link for an up-to-date comparison between the source branch of this pull request and the master branch.
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 the ready label Aug 6, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 3, 2021

@dcubed-ojdk This pull request has been inactive for more than 4 weeks and will be automatically closed if another 4 weeks passes without any activity. To avoid this, simply add a new comment to the pull request. Feel free to ask for assistance if you need help with progressing this pull request towards integration!

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 11, 2021

Rebased this project to the latest jdk/jdk baseline as of 2021.10.11 in the afternoon.

This PR needs a review from either @mgronlun or @egahlin since I'm changing
JFR code here.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 11, 2021

The rebase has passed Mach5 Tier1 testing; Mach5 Tier[23] are in process.

Update: Mach5 Tier[23] test also passed.

Copy link

@mgronlun mgronlun left a comment

Looks good Dan, thank you.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Oct 12, 2021

@sspitsyn and @mgronlun - Thanks for the reviews.

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 2021

Going to push as commit 8657f77.
Since your change was applied there have been 17 commits pushed to the master branch:

  • b8bd259: 8271737: Only normalize the cached user.dir property once
  • 89999f7: 8275131: Exceptions after a touchpad gesture on macOS
  • 07b1f1c: 8274548: (fc) FileChannel gathering write fails with IOException "Invalid argument" on macOS 11.6
  • f623460: 8274911: testlibrary_tests/ir_framework/tests/TestIRMatching.java fails with "java.lang.RuntimeException: Should have thrown exception"
  • e393c5e: 8275074: Cleanup unused code in JFR LeakProfiler
  • e16b93a: 8274770: [PPC64] resolve_jobject needs a generic implementation to support load barriers
  • 1ab6414: 8275051: Shenandoah: Correct ordering of requested gc cause and gc request flag
  • b460d6d: 8275091: /src/jdk.management.jfr/share/classes/module-info.java has non-canonical order
  • d04d4ee: 8274894: Use Optional.empty() instead of ofNullable(null) in HttpResponse.BodySubscribers.discarding
  • 33050f8: 8274986: max code printed in hs-err logs should be configurable
  • ... and 7 more: https://git.openjdk.java.net/jdk/compare/829dea45c9ab90518f03a66aad7e681cd4fda8b3...master

Your commit was automatically rebased without conflicts.

@openjdk openjdk bot closed this Oct 12, 2021
@openjdk openjdk bot added integrated and removed ready rfr labels Oct 12, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Oct 12, 2021

@dcubed-ojdk Pushed as commit 8657f77.

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

@dcubed-ojdk dcubed-ojdk deleted the JDK-8271514 branch Oct 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated serviceability
6 participants