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

8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes #4677

Closed
wants to merge 14 commits into from

Conversation

dcubed-ojdk
Copy link
Member

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

A fix to reduce ThreadsListHandle overhead in relation to handshakes and
we add sanity checks for ThreadsListHandles higher in the call stack.

This fix was tested with Mach5 Tier[1-8]; Tier8 is still running.


Progress

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

Issue

  • JDK-8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes

Reviewers

Reviewing

Using git

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

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

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4677

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

Using diff file

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

@dcubed-ojdk
Copy link
Member Author

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

/label add hotspot-runtime
/label add serviceability

@dcubed-ojdk dcubed-ojdk marked this pull request as ready for review Jul 3, 2021
@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Jul 3, 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 openjdk bot added rfr hotspot-runtime labels Jul 3, 2021
@openjdk
Copy link

@openjdk openjdk bot commented Jul 3, 2021

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

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

@openjdk openjdk bot commented Jul 3, 2021

@dcubed-ojdk
The serviceability label was successfully added.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Jul 3, 2021

@dholmes-ora
Copy link
Member

@dholmes-ora dholmes-ora commented Jul 4, 2021

Hi Dan,

I just updated the bug report. This really isn't addressing the reasons the RFE was filed.

David

@bridgekeeper
Copy link

@bridgekeeper bridgekeeper bot commented Sep 10, 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!

Copy link
Contributor

@coleenp coleenp left a comment

This looks good.

@openjdk
Copy link

@openjdk openjdk bot commented Sep 22, 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:

8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes

Reviewed-by: coleenp, sspitsyn, dholmes, rehn

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

  • fc0fe25: 8273235: tools/launcher/HelpFlagsTest.java Fails on Windows 32bit
  • 3934fe5: 8275847: Scheduling fails with "too many D-U pinch points" on small method
  • 44047f8: 8274328: C2: Redundant CFG edges fixup in block ordering
  • 8849154: 8275846: read_base_archive_name() could read past the end of buffer
  • 2653cfb: 8276688: Remove JLinkReproducibleXXXTest from ProblemList.txt
  • c7095b2: 8276207: Properties.loadFromXML/storeToXML works incorrectly for supplementary characters
  • ed7ecca: 8254739: G1: Optimize evacuation failure for regions with few failed objects
  • 59c3dcc: 8276746: Add section on reproducible builds in building.md
  • 0e0dd33: 8276129: PretouchTask should page-align the chunk size
  • a472433: 8276572: Fake libsyslookup.so library causes tooling issues
  • ... and 80 more: https://git.openjdk.java.net/jdk/compare/c8abe354c1ddc988ff54b9a96a4a825e2aa70f4b...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 the ready label Sep 22, 2021
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Sep 22, 2021

@coleenp - Thanks, but...

I'm redoing this fix in a different way to address @dholmes-ora comment from above:

I just updated the bug report. This really isn't addressing the reasons the RFE was filed.

Copy link
Contributor

@sspitsyn sspitsyn left a comment

Hi Dan,
It looks good.
Thanks,
Serguei

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

HOWEVER, I'm still working on the latest version of the fix so please do not review
this PR yet.

@dcubed-ojdk dcubed-ojdk changed the title 8249004: Reduce ThreadListHandle overhead in relation to direct handshakes 8249004: Reduce ThreadsListHandle overhead in relation to direct handshakes Oct 14, 2021
@openjdk openjdk bot removed the ready label Oct 14, 2021
@dcubed-ojdk
Copy link
Member Author

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

@dholmes-ora, @coleenp and @sspitsyn - I've finished reworking the patch
and I'm finally happy with how it works. This version has passed Mach5 Tier[1-7]
and has mostly finished Mach5 Tier8 (it's in the 24-hour test phase)...

Update Mach5 Tier8 also passed.

@dcubed-ojdk
Copy link
Member Author

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

/issue JDK-8249004

@openjdk
Copy link

@openjdk openjdk bot commented Oct 14, 2021

@dcubed-ojdk This issue is referenced in the PR title - it will now be updated.

@openjdk openjdk bot added the ready label Oct 14, 2021
Copy link
Contributor

@coleenp coleenp left a comment

This has more moving pieces than the last version. I'm a bit uneasy about passing NULL as a thread to Handshake::execute(). This shouldn't be something that should happen.

target->handshake_state()->add_operation(&op);
bool target_is_dead = false;
if (target == nullptr) {
target_is_dead = true;
Copy link
Contributor

@coleenp coleenp Oct 15, 2021

Choose a reason for hiding this comment

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

Why would you pass a NULL target thread to Handshake::execute? Why would the caller not check if the target is dead?

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 15, 2021

Choose a reason for hiding this comment

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

The NULL target thread being passed in is actually handled by the baseline code:

  ThreadsListHandle tlh;
  if (tlh.includes(target)) {

tlh.includes(target) returns false when target is NULL/nullptr.
I just made the already handled situation more explicit.

Why would the caller not check if the target is dead?

Hmmm... It's hard for me to answer that question since I didn't write
the original code. The test code that calls WB_HandshakeWalkStack()
or WB_AsyncHandshakeWalkStack() can call those functions with
a thread_handle that translates into a thread_oop that returns a
NULL JavaThread*.

See the comment that I added to WB_AsyncHandshakeWalkStack() above.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 16, 2021

Choose a reason for hiding this comment

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

Update: I've added comments to WB_HandshakeReadMonitors() and
WB_HandshakeWalkStack() to clarify their expectations.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 1, 2021

Choose a reason for hiding this comment

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

Update again: I took a closer look at WB_AsyncHandshakeWalkStack(),
WB_HandshakeReadMonitors() and WB_HandshakeWalkStack() and
I realized that those functions were not properly converting a jobject into
a protected JavaThread*. I've updated them to call the proper conversion
function and I've changed this code to guarantee() that the target is not
nullptr.

Copy link
Contributor

@coleenp coleenp Nov 5, 2021

Choose a reason for hiding this comment

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

Great.

// the calling thread in order to verify proper ThreadsListHandle
// placement somewhere in the calling context.
bool Thread::is_JavaThread_protected_by_my_ThreadsList(const JavaThread* p) {
Thread* current_thread = Thread::current();
Copy link
Contributor

@coleenp coleenp Oct 15, 2021

Choose a reason for hiding this comment

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

Shouldn't you call this on the current thread as "this" argument?

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 15, 2021

Choose a reason for hiding this comment

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

I modeled the new check after the existing:

bool Thread::is_JavaThread_protected(const JavaThread* p) {

which is also a static function.

Copy link
Member

@dholmes-ora dholmes-ora Oct 17, 2021

Choose a reason for hiding this comment

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

While the name is somewhat ungainly - and unnecessarily detailed given is_JavaThread_protected has a similar constraint - it should be a static function as given because it must only be called on the current thread, and an instance method would give the false impression that it could be called on any thread.

That said it should be possible to write that code block only once and reuse it. And the name as I said is somewhat ungainly. You could even have:

static bool is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly = false) {

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Oct 29, 2021

Choose a reason for hiding this comment

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

I'm checking out adding the checkTLHOnly param to is_JavaThread_protected()
and replacing is_JavaThread_protected_by_my_ThreadsList() calls with calls
to the updated function.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 1, 2021

Choose a reason for hiding this comment

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

Testing went well so the above change will be in the next change that I push.

log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, no suspension", p2i(this));
guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
"missing ThreadsListHandle in calling context.");
if (is_exiting()) {
Copy link
Contributor

@coleenp coleenp Oct 15, 2021

Choose a reason for hiding this comment

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

So suspend_thread and resume thread's caller already takes a ThreadsListHandle so this is unnecessary and never happens.

@dcubed-ojdk
Copy link
Member Author

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

This looks promising but I'm unclear on some of the details. I can't quite work out
the criteria for deciding when to pass the TLH through to Handshake::execute. If
it is passed through then the target is checked for being alive ie covered by the
TLH. But if the TLH is null then it is assumed that the target is protected elsewhere
up the stack and also assumed to be alive. I'm not sure why the two must go
together. For example given this code:

    ThreadsListHandle tlh(current_thread);
    JavaThread *java_thread;
    err = JvmtiExport::cv_external_thread_to_JavaThread(tlh.list(), *thread_list, &java_thread, NULL);
    GetSingleStackTraceClosure op(this, current_thread, *thread_list, max_frame_count);
    Handshake::execute(&op, &tlh, java_thread);

what difference does it make if instead the last line is just:

Handshake::execute(&op, java_thread);

? How do I know which form should be used?

Hmmm... This is a really good observation. When I was putting together this
patch (the second time), my criteria was fairly simple: if we were calling
Handshake::execute() and we happened to have a ThreadsListHandle
in hand, then I switched to the new version of Handshake::execute() that
takes a ThreadsListHandle*.

As for what difference does it make, when we pass in a ThreadsListHandle,
then we're saying "This is the ThreadsListHandle that protects the target
JavaThread." and that's the only ThreadsListHandle that we search. If we
call the Handshake::execute() form that does not take a ThreadsListHandle,
then we might end up searching all of the ThreadsListHandles that are
associated with the calling thread. Since we stop on first match, we might only
search the first one, but if the target JavaThread isn't in the first one, then we'll
continue to search the next one and so on.

Of course, down the road, if we end up changing code like this:

guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(target),
                  "missing ThreadsListHandle in calling context.");

into an assert(), then we won't be doing any search in 'release' bits and then
the Handshake::execute() that takes a ThreadsListHandle* will be more
expensive in release bits.

@mlbridge
Copy link

@mlbridge mlbridge bot commented Oct 17, 2021

Mailing list message from David Holmes on hotspot-runtime-dev:

On 16/10/2021 8:26 am, Daniel D.Daugherty wrote:

On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes <dholmes at openjdk.org> wrote:

Daniel D. Daugherty has updated the pull request incrementally with one additional commit since the last revision:

8249004.cr1.patch

src/hotspot/share/prims/jvmtiEventController.cpp line 623:

621: // If we have a JvmtiThreadState, then we've reached the point where
622: // threads can exist so create a ThreadsListHandle to protect them.
623: ThreadsListHandle tlh;

Good catch on the missing TLH for this code.

It wasn't quite missing from the baseline code. This version of execute():

`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`

used to always create a ThreadsListHandle. I added a `ThreadsListHandle*`
parameter to that version and created a wrapper with the existing signature
to pass `nullptr` to the execute() version with the `ThreadsListHandle*`
parameter. What that means is that all existing callers of:

`Handshake::execute(HandshakeClosure* hs_cl, JavaThread* target)`

no longer had a ThreadsListHandle created for them. With the new sanity
check in place, I shook the trees to make sure that we had explicit
ThreadsListHandles in place for the locations that needed them.

`JvmtiEventControllerPrivate::recompute_enabled()` happened to be
one of the places where the ThreadsListHandle created by execute()
was hiding the fact that `recompute_enabled()` needed one.

Yup and that is exactly why I said good catch on finding the missing TLH.

Cheers,
David

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 2, 2021

@coleenp and @dholmes-ora, the latest version should address the last
of your previous comments. Please re-review when you get the chance.

@robehn and @sspitsyn - It would be good to get re-reviews from you guys
on this latest version. Thanks!

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Hi Dan,

Generally seems okay but a couple of minor issues below.

Thanks,
David

Handshake::execute(hs_cl, nullptr, target);
}

void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh_p, JavaThread* target) {
Copy link
Member

@dholmes-ora dholmes-ora Nov 3, 2021

Choose a reason for hiding this comment

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

Nit: can we drop the _p part of tlh_p please.

Copy link
Contributor

@robehn robehn Nov 3, 2021

Choose a reason for hiding this comment

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

Yes, please.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 3, 2021

Choose a reason for hiding this comment

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

Fixed in handshake.[ch]pp.

bool Thread::is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly) {
Thread* current_thread = nullptr;
if (checkTLHOnly) {
current_thread = Thread::current();
Copy link
Member

@dholmes-ora dholmes-ora Nov 3, 2021

Choose a reason for hiding this comment

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

This seems redundant due to line 463. You can just have a if (!checkTLHOnly) block here.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 3, 2021

Choose a reason for hiding this comment

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

I suspect that the way that git is displaying the diffs is confusing you.

We need current_thread set if we get to line 474 so we have to init
current_thread on line 446 for the checkTLHOnly == true case and
on line 463 for the checkTLHOnly == false case.

I could simplify the logic by always setting current thread when it is
declared on 444, but I was trying to avoid the call to Thread::current()
until I actually needed it. I thought Thread::current() can be expensive.
Is this no longer the case?

Copy link
Member

@dholmes-ora dholmes-ora Nov 4, 2021

Choose a reason for hiding this comment

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

Sorry I missed that line 463 is still within the else from line 447.

Thread::current() is a compiler-defined thread-local access so should be relatively cheap these days, but I have no numbers.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 4, 2021

Choose a reason for hiding this comment

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

I'm really tempted to go ahead and change it to always set
current thread when it is declared and then clean things up a bit.

Copy link
Contributor

@coleenp coleenp Nov 4, 2021

Choose a reason for hiding this comment

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

Yes, ^ that might make the logic easier to follow. I can't figure out what checkTLSOnly means. Could it be refactored into a different function like check_TLS() and then call it in the place where you pass true instead of is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks?

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 5, 2021

Choose a reason for hiding this comment

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

I've changed is_JavaThread_protected() to materialize current_thread when it is declared
to simplify the code paths.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 5, 2021

Choose a reason for hiding this comment

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

I can't figure out what checkTLSOnly means.

See the function's header comment:

// ... If checkTLHOnly is true (default is false), then we only check
// if the target JavaThread is protected by a ThreadsList (if any) associated
// with the calling Thread.

Could it be refactored into a different function like check_TLS() and then
call it in the place where you pass true instead of is_JavaThread_protected?

I had a copy of this logic in a separate function and @dholmes-ora suggested
that I add the checkTLHOnly parameter to this function and get rid of the
separate copy so I've been there and done that.

I could move the logic into a separate function and then call that new function
from here and from the other call sites that pass true, but then I'd have to
materialize current_thread in that new function and then on this code path we
would be materializing current_thread twice. I don't want to do that.

Does checkTLSOnly skip a lot of these checks?

Please read the header comment to see what checkTLHOnly does.

log_trace(thread, suspend)("JavaThread:" INTPTR_FORMAT " not on ThreadsList, nothing to resume", p2i(this));
guarantee(Thread::is_JavaThread_protected(this, /* checkTLHOnly */ true),
"missing ThreadsListHandle in calling context.");
if (is_exiting()) {
Copy link
Member

@dholmes-ora dholmes-ora Nov 3, 2021

Choose a reason for hiding this comment

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

Can't we remove this the same as we did for java_suspend()?

Copy link
Contributor

@robehn robehn Nov 3, 2021

Choose a reason for hiding this comment

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

Yes, please

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 3, 2021

Choose a reason for hiding this comment

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

The rationale for removing the is_exiting() check from java_suspend() was that it
was redundant because the handshake code detected and handled the is_exiting()
case so we didn't need to do that work twice.

If we look at HandshakeState::resume() there is no logic for detecting or handling
the possibility of an exiting thread. That being said, we have to look closer at what
HandshakeState::resume() does and whether that logic can be harmful if executed
on an exiting thread.

Here's the code:

bool HandshakeState::resume() {
  if (!is_suspended()) {
    return false;
  }
  MutexLocker ml(&_lock, Mutex::_no_safepoint_check_flag);
  if (!is_suspended()) {
    assert(!_handshakee->is_suspended(), "cannot be suspended without a suspend request");
    return false;
  }
  // Resume the thread.
  set_suspended(false);
  _lock.notify();
  return true;
}

I'm not seeing anything in HandshakeState::resume() that
worries me with respect to an exiting thread. Of course, the
proof is in the testing so I'll rerun the usual testing after
deleting that code.

Copy link
Member

@dholmes-ora dholmes-ora Nov 4, 2021

Choose a reason for hiding this comment

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

A suspended thread cannot be exiting - else the suspend logic is broken. So, given you can call resume() on a not-suspended thread, as long as the handshake code checks for is_supended() (which it does) then no explicit is_exiting check is needed.

Copy link
Member Author

@dcubed-ojdk dcubed-ojdk Nov 4, 2021

Choose a reason for hiding this comment

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

Agreed! I have to keep reminding myself that with handshake based suspend
and resume, we just don't have the same races with exiting threads that we
used to have.

robehn
robehn approved these changes Nov 3, 2021
Handshake::execute(hs_cl, nullptr, target);
}

void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle* tlh_p, JavaThread* target) {
Copy link
Contributor

@robehn robehn Nov 3, 2021

Choose a reason for hiding this comment

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

Yes, please.

@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 4, 2021

@dholmes-ora and @robehn - Thanks for your reviews on the previous version
(8249004.cr2.patch). Mach5 Tier[1-5] testing has finished and looks good;
Mach5 Tier[678] are still running.

Update: Tier[67] are finished and looking good. Tier8 is down to the 24 hour tasks.

Copy link
Contributor

@coleenp coleenp left a comment

Sorry for such a long delay looking at this. I had a couple questions and a suggestion.

if (state != nullptr) {
// If we have a JvmtiThreadState, then we've reached the point where
// threads can exist so create a ThreadsListHandle to protect them.
ThreadsListHandle tlh;
Copy link
Contributor

@coleenp coleenp Nov 4, 2021

Choose a reason for hiding this comment

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

Should the ThreadsListHandle protect JvmtiThreadState::first also? If state->next() needs it why doesn't the first entry need this? There's no atomic load on the _head field.

bool Thread::is_JavaThread_protected(const JavaThread* p, bool checkTLHOnly) {
Thread* current_thread = nullptr;
if (checkTLHOnly) {
current_thread = Thread::current();
Copy link
Contributor

@coleenp coleenp Nov 4, 2021

Choose a reason for hiding this comment

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

Yes, ^ that might make the logic easier to follow. I can't figure out what checkTLSOnly means. Could it be refactored into a different function like check_TLS() and then call it in the place where you pass true instead of is_JavaThread_protected? Does checkTLSOnly skip a lot of these checks?

@dcubed-ojdk
Copy link
Member Author

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

@coleenp - Thanks for the re-review!

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

@coleenp coleenp left a comment

Thanks for the offline comments. I still think refactoring is_JavaThread_protected would be a lot nicer but that's my opinion and you don't have to change it.

if (state != nullptr) {
// If we have a JvmtiThreadState, then we've reached the point where
// threads can exist so create a ThreadsListHandle to protect them.
ThreadsListHandle tlh;
Copy link
Contributor

@coleenp coleenp Nov 5, 2021

Choose a reason for hiding this comment

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

JvmtiThreadState objects point to JavaThread and vice versa, so I still don't see why you don't protect the first element.

@dcubed-ojdk
Copy link
Member Author

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

@dholmes-ora and @sspitsyn - I still need a re-review from you guys.

@robehn - A re-review from you would be good, but I think I cleanly
addressed your last comments...

@dcubed-ojdk
Copy link
Member Author

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

JvmtiThreadState objects point to JavaThread and vice versa, so I still don't see why you don't protect the first element.

I've written up a rather long analysis about how the use of JvmtiThreadState_lock
in JvmtiEventControllerPrivate::recompute_enabled() means that we can safely
traverse the JvmtiThreadState list returned by JvmtiThreadState::first() without
racing with an exiting JavaThread. I've sent it to you via direct email.

I could amend the comment above the ThreadsListHandle like this:

    // If we have a JvmtiThreadState, then we've reached the point where
    // threads can exist so create a ThreadsListHandle to protect them.
    // The held JvmtiThreadState_lock prevents exiting JavaThreads from
    // being removed from the JvmtiThreadState list we're about to walk
    // so this ThreadsListHandle exists just to satisfy the lower level sanity
    // checks that the target JavaThreads are protected.
    ThreadsListHandle tlh;

@dcubed-ojdk
Copy link
Member Author

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

Updated comment has been pushed to this PR.

For some reason, the comments that @coleenp and I are posting in the
"Files changed" view are not showing up here.

Copy link
Member

@dholmes-ora dholmes-ora left a comment

Thanks Dan - updates look good.

David

robehn
robehn approved these changes Nov 8, 2021
@dcubed-ojdk
Copy link
Member Author

@dcubed-ojdk dcubed-ojdk commented Nov 8, 2021

@dholmes-ora and @robehn - Thanks for the re-reviews!

/integrate

@openjdk
Copy link

@openjdk openjdk bot commented Nov 8, 2021

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

  • c815c5c: 8276209: Some call sites doesn't pass the parameter 'size' to SharedRuntime::dtrace_object_alloc(_base)
  • 71c4b19: 8276562: Fix to JDK-8263155 left out the help text changes
  • cc2cac1: 8274686: java.util.UUID#hashCode() should use Long.hashCode()
  • 0c2d00b: 8275097: Wrong span of the 'default' tag
  • fa754b8: 8276149: jshell throws EOF error when throwing exception inside switch expression
  • 4c14edd: 8274734: the method jdk.jshell.SourceCodeAnalysis documentation not working
  • ff6863c: 8276540: Howl Full CardSet container iteration marks too many cards
  • 5448139: 8271056: C2: "assert(no_dead_loop) failed: dead loop detected" due to cmoving identity
  • 0395e4e: 8276768: Snippet copy feature should use button instead of link
  • d8b0dee: 8276239: Better tables in java.util.random package summary
  • ... and 90 more: https://git.openjdk.java.net/jdk/compare/c8abe354c1ddc988ff54b9a96a4a825e2aa70f4b...master

Your commit was automatically rebased without conflicts.

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

@openjdk openjdk bot commented Nov 8, 2021

@dcubed-ojdk Pushed as commit ea23e73.

💡 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-8249004 branch Nov 8, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
hotspot-runtime integrated serviceability
5 participants