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

8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator #4948

Closed
wants to merge 8 commits into from

Conversation

@dcubed-ojdk
Copy link
Member

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

A trivial fix to support JavaThreadIteratorWithHandle replacement by 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-8271513: support JavaThreadIteratorWithHandle replacement by 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/4948/head:pull/4948
$ git checkout pull/4948

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4948

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

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4948.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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Jul 30, 2021

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

  • hotspot-runtime

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.

Loading

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

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

/contributor add @kimbarrett

Loading

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

@openjdk openjdk bot commented Jul 30, 2021

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

Loading

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 30, 2021

Webrevs

Loading

Copy link

@kimbarrett kimbarrett left a comment

Looks good, and trivial.

Loading

@dcubed-ojdk
Copy link
Member Author

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

@kimbarrett - Thanks for the review!

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Aug 1, 2021

Since @kimbarrett is listed as the Contributor, I need another (R)eviewer.
I can't be listed as a (R)eviewer because I created the PR. Any takers?

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Sorry Dan, while I like the use of the for-each loops very much I don't understand the details of the iterator declaration. I definitely do not classify this as a trivial change.

Thanks,
David

Loading

@@ -328,6 +328,10 @@ class ThreadsListHandle : public StackObj {
return _list_ptr.list();
}

using Iterator = ThreadsList::Iterator;
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.

Is this like a typedef? I don't think I want to see plain "Iterator" and not know what the actual type is here.

Loading

Choose a reason for hiding this comment

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

It's a type alias, which in this simple form is just like a typedef. Type aliases can also have template parameters. The iterator type used for iterating over a ThreadsListHandle today happens to be the same type as used for iterating over the underlying ThreadsList. There's no particular reason to wire that information in to clients.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Aug 3, 2021

Choose a reason for hiding this comment

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

So ThreadsListHandle::Iterator is a global(?) alias for ThreadsList::Iterator?

If find this name/type aliasing somewhat obscure and confusing. I mean ideally the thing returned would just be an Iterator and the details of whether it is a Foo::Iterator or Bar::Iterator would not be evident in the API.

Loading

@@ -107,6 +107,9 @@ inline void ThreadsList::threads_do(T *cl) const {
}
}

ThreadsListHandle::Iterator ThreadsListHandle::begin() { return list()->begin(); }
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.

I don't understand this - why is the return type not ThreadsList::Iterator as per the declaration in the hpp file? That using directive is apparently doing something more bizarre than I thought.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Aug 3, 2021

Choose a reason for hiding this comment

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

See above. I think I understand this now. Don't like it; but understand it.

Loading

Choose a reason for hiding this comment

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

I'm not sure what declaration in the hpp file you are referring to. The function declaration specifies a return type of (unqualified) Iterator which is declared (via the type alias) in that class. Outside the scope of that class it needs to be qualified, i.e. ThreadsListHandle::Iterator.

There's nothing bizarre about it. Maybe this looks unfamiliar because nested types are relatively uncommon in HotSpot, and even more so using such in a return type for a definition outside the body of the class.

The definition can't use unqualified Iterator because that wouldn't compile. The leading return type isn't in the scope of the class of the member, so must itself be qualified too. It's qualified by the class of the member, so that it refers to the name in that class. The scoping rules for the return type of a member function are an annoying bit of C++ syntax that can be dealt with differently using C++11 trailing return types , but that's a not yet discussed feature in the style guide. That is, this could have been written as
auto ThreadsListHandle::begin() -> Iterator { return list()->begin(); }
(note the arrow pointing to the return type, which doesn't need qualification because the trailing return type is in the scope of ThreadsListHandle). Or it can be even shorter using type deduction
auto ThreadsListHandle::begin() { return list()->begin(); }
though I'm not sure type deduction helps readability here.

Loading

Copy link
Member

@dholmes-ora dholmes-ora Aug 4, 2021

Choose a reason for hiding this comment

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

@kimbarrett not knowing how the aliasing works I was looking at it more as a textual substitution so ThreadsListHandle::Iterator looked to me like it would "expand" to ThreadsListHandler::ThreadsList::Iterator (which makes no sense) and I was not reading plain "Iterator" as ThreadsListHandle::Iterator.
I don't see why the ThreadsListHandle methods couldn't just return ThreadsList::Iterator as we know a ThreadsListHandle is just a wrapper for a ThreadsList. I don't think that is something we have to worry about "wiring" into clients. As I said the right style here would have an abstract Iterator type and we wouldn't care about the concrete implementation type. YMMV.

Loading

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

@openjdk-notifier openjdk-notifier bot commented Aug 2, 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-8193559
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

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 2, 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:

8271513: support JavaThreadIteratorWithHandle replacement by new ThreadsList::Iterator

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

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

  • adb0ae5: 8261441: JFR: Filename expansion
  • e38e365: 8271208: Typo in ModuleDescriptor.read javadoc
  • 14692d5: Merge
  • dfacda4: 8270872: Final nroff manpage update for JDK 17
  • 90f85ff: 8271588: JFR Recorder Thread crashed with SIGSEGV in write_klass
  • f312f28: 8271863: ProblemList serviceability/sa/TestJmapCore.java on linux-x64 with ZGC
  • 62e72ad: 8271293: Monitor class should use ThreadBlockInVMPreprocess
  • cb36880: 8270116: Expand ButtonGroupLayoutTraversalTest.java to run in all LaFs, including Aqua on macOS
  • 3ab95d1: 8271905: mark hotspot runtime/Metaspace tests which ignore external VM flags
  • e2c5bfe: 8271308: (fc) FileChannel.transferTo() transfers no more than Integer.MAX_VALUE bytes in one call
  • ... and 59 more: https://git.openjdk.java.net/jdk/compare/0a85236396c667c8d2c890e4384c623b39455075...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.

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 2, 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).

Loading

@openjdk openjdk bot added the ready label Aug 2, 2021
@@ -854,7 +854,7 @@ void JavaThread::collect_counters(jlong* array, int length) {
for (int i = 0; i < length; i++) {
array[i] = _jvmci_old_thread_counters[i];
}
for (JavaThreadIteratorWithHandle jtiwh; JavaThread *tp = jtiwh.next(); ) {
for (JavaThread* tp : ThreadsListHandle()) {
Copy link
Contributor

@coleenp coleenp Aug 3, 2021

Choose a reason for hiding this comment

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

This is a nicer interface with underlying C++ magic. There are lots of these in the source code. Is it the intention to replace these as we go?

Loading

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Aug 5, 2021

Choose a reason for hiding this comment

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

There's definitely lots of these:

$ grep -r JavaThreadIteratorWithHandle src/hotspot | sed 's/:.*//' | sort | uniq -c
   1 src/hotspot/os/linux/os_linux.cpp
   1 src/hotspot/share/compiler/compileBroker.cpp
   1 src/hotspot/share/gc/parallel/mutableNUMASpace.cpp
   2 src/hotspot/share/gc/shared/collectedHeap.cpp
   1 src/hotspot/share/gc/shared/gcLocker.cpp
   1 src/hotspot/share/gc/shenandoah/shenandoahCodeRoots.cpp
   5 src/hotspot/share/gc/shenandoah/shenandoahHeap.cpp
   2 src/hotspot/share/gc/shenandoah/shenandoahPacer.cpp
   1 src/hotspot/share/jfr/utilities/jfrThreadIterator.cpp
   1 src/hotspot/share/jfr/utilities/jfrThreadIterator.hpp
   1 src/hotspot/share/opto/idealGraphPrinter.cpp
   1 src/hotspot/share/prims/jvmtiEnvBase.cpp
   1 src/hotspot/share/prims/jvmtiEventController.cpp
   1 src/hotspot/share/prims/jvmtiImpl.cpp
   1 src/hotspot/share/prims/jvmtiTagMap.cpp
   1 src/hotspot/share/prims/whitebox.cpp
   5 src/hotspot/share/runtime/escapeBarrier.cpp
   2 src/hotspot/share/runtime/handshake.cpp
   1 src/hotspot/share/runtime/os.cpp
   8 src/hotspot/share/runtime/safepoint.cpp
   1 src/hotspot/share/runtime/thread.cpp
   3 src/hotspot/share/runtime/threadSMR.hpp
   3 src/hotspot/share/runtime/vmOperations.cpp
   1 src/hotspot/share/services/heapDumper.cpp
   1 src/hotspot/share/services/management.cpp
   1 src/hotspot/share/services/threadService.cpp
   1 src/hotspot/share/utilities/globalCounter.cpp

I believe the plan is to migrate to this newer, cleaner interface.
Obviously not with this bug, but down the road...

Loading

Copy link
Contributor

@coleenp coleenp Aug 5, 2021

Choose a reason for hiding this comment

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

Awesome, thanks!

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Aug 5, 2021

@kimbarrett, @dholmes-ora and @coleenp - I'm not seeing any commentary in this PR
indicating that there are any agreed up changes to make here. I'm going to double
check my email folder for this bug ID since I know that not everything sent via email
has made it into the PR...

So far, only @kimbarrett has marked this PR as reviewed. However, since this code is
actually his, that doesn't count. My own review doesn't count since I created the PR
so if we don't get another (R)eviewer approval, then this PR won't be moving forward.

Loading

coleenp
coleenp approved these changes Aug 5, 2021
Copy link
Contributor

@coleenp coleenp left a comment

Looks good! Sorry for the delay, I didn't see the answer to my question at first.

Loading

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Changes are fine now I have a better understanding of the aliasing. Not sure it was really needed but ...

Thanks,
David

Loading

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Aug 6, 2021

@coleenp and @dholmes-ora - Thanks for closing the loop on this review thread.

/integrate

Loading

@openjdk
Copy link

@openjdk openjdk bot commented Aug 6, 2021

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

Your commit was automatically rebased without conflicts.

Loading

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

@openjdk openjdk bot commented Aug 6, 2021

@dcubed-ojdk Pushed as commit fa36e33.

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

Loading

@dcubed-ojdk dcubed-ojdk deleted the JDK-8271513 branch Aug 6, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
4 participants