On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote:
>> 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.
>
> Hi Dan,
>
> I
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Fri, 5 Nov 2021 21:54:01 GMT, Coleen Phillimore wrote:
>> 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
On Fri, 5 Nov 2021 23:42:07 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
On Fri, 5 Nov 2021 20:46:21 GMT, Daniel D. Daugherty wrote:
>> 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
On Fri, 5 Nov 2021 15:43:37 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Fri, 5 Nov 2021 16:43:04 GMT, Coleen Phillimore wrote:
>> The `ThreadsListHandle` protects `JavaThread` objects not `JvmtiThreadState`
>> objects.
>> `JvmtiThreadState::first()` returns the head of the global list of
>> `JvmtiThreadState`
>> objects for the system. Each `JvmtiThreadState`
On Fri, 5 Nov 2021 15:43:37 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote:
>> 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.
>
> Hi Dan,
>
> I
On Fri, 5 Nov 2021 15:38:27 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> The `ThreadsListHandle` protects
On Mon, 1 Nov 2021 15:59:58 GMT, Daniel D. Daugherty wrote:
>> Update: I've added comments to WB_HandshakeReadMonitors() and
>> WB_HandshakeWalkStack() to clarify their expectations.
>
> Update again: I took a closer look at `WB_AsyncHandshakeWalkStack()`,
> `WB_HandshakeReadMonitors()` and
On Thu, 4 Nov 2021 21:45:16 GMT, Coleen Phillimore wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> Sorry for such a long delay looking at this. I had a couple questions and a
>
On Fri, 5 Nov 2021 15:15:14 GMT, Daniel D. Daugherty wrote:
>> 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
On Thu, 4 Nov 2021 19:33:33 GMT, Coleen Phillimore wrote:
>> 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*`
>>
On Thu, 4 Nov 2021 21:44:45 GMT, Coleen Phillimore wrote:
>> 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.
>
> Yes, ^ that might make the logic easier to follow. I can't figure out what
> checkTLSOnly means.
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
On Thu, 4 Nov 2021 17:02:59 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> I'm really tempted to go
On Fri, 15 Oct 2021 21:34:50 GMT, Daniel D. Daugherty
wrote:
>> 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.
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Thu, 4 Nov 2021 01:16:03 GMT, David Holmes wrote:
>> 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
On Thu, 4 Nov 2021 01:18:23 GMT, David Holmes wrote:
>> 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
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote:
>> 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.
>
> Hi Dan,
>
> I
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
On Wed, 3 Nov 2021 16:05:21 GMT, Daniel D. Daugherty wrote:
>> Yes, please
>
> 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
On Wed, 3 Nov 2021 15:45:06 GMT, Daniel D. Daugherty wrote:
>> src/hotspot/share/runtime/thread.cpp line 446:
>>
>>> 444: Thread* current_thread = nullptr;
>>> 445: if (checkTLHOnly) {
>>> 446: current_thread = Thread::current();
>>
>> This seems redundant due to line 463. You can just
On Wed, 3 Nov 2021 09:50:08 GMT, Robbin Ehn wrote:
>> src/hotspot/share/runtime/handshake.cpp line 350:
>>
>>> 348: }
>>> 349:
>>> 350: void Handshake::execute(HandshakeClosure* hs_cl, ThreadsListHandle*
>>> tlh_p, JavaThread* target) {
>>
>> Nit: can we drop the `_p` part of `tlh_p` please.
On Wed, 3 Nov 2021 01:21:50 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/thread.cpp line 446:
>
>> 444: Thread* current_thread =
On Wed, 3 Nov 2021 01:19:21 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr2.patch
>
> src/hotspot/share/runtime/handshake.cpp line 350:
>
>> 348: }
>> 349:
>> 350: void
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Tue, 2 Nov 2021 17:26:41 GMT, Daniel D. Daugherty wrote:
>> 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.
>
> Daniel D.
On Fri, 15 Oct 2021 18:31:26 GMT, Coleen Phillimore wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> This has more moving pieces than the last version. I'm a bit uneasy about
>
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
On Mon, 1 Nov 2021 01:55:48 GMT, David Holmes wrote:
>> U... The purpose of the new `is_exiting()` check and the baseline's
>> `ThreadsListHandle::includes()` check is to avoid making this call:
>>
>> return this->handshake_state()->suspend();
>>
>> The call we are avoiding is the one
On Sat, 16 Oct 2021 15:58:21 GMT, Daniel D. Daugherty
wrote:
>> 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
On Fri, 29 Oct 2021 22:16:17 GMT, Daniel D. Daugherty
wrote:
>> 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
> 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.
Daniel D. Daugherty has updated the pull request with a new target base due to
On Fri, 29 Oct 2021 22:35:50 GMT, Daniel D. Daugherty
wrote:
>> The `is_exiting` check seems unnecessary as the handshake code will not
>> handshake with an exiting thread. The nested TLH was unnecessary too AFAICS.
>
> U... The purpose of the new `is_exiting()` check and the baseline's
>
On Sun, 17 Oct 2021 12:52:15 GMT, David Holmes wrote:
>> On rereading all of these comments and the current baseline code, I have
>> to clarify one thing:
>>
>> There is a minor change in behavior caused by switching from a
>> `ThreadsListHandle::includes()` check to a
On Sun, 17 Oct 2021 12:45:59 GMT, David Holmes wrote:
>> I modeled the new check after the existing:
>>
>>
>> bool Thread::is_JavaThread_protected(const JavaThread* p) {
>>
>>
>> which is also a static function.
>
> While the name is somewhat ungainly - and unnecessarily detailed given
>
On Sat, 16 Oct 2021 16:21:08 GMT, Daniel D. Daugherty
wrote:
>>> This seems an unrelated change in behaviour ??
>>
>> Actually this is equivalent code. The baseline code does this:
>>
>> ThreadsListHandle tlh;
>> if (!tlh.includes(this)) {
>> log_trace(thread, suspend)("JavaThread:"
On Fri, 15 Oct 2021 22:04:16 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/runtime/thread.cpp line 497:
>>
>>> 495: // placement somewhere in the calling context.
>>> 496: bool Thread::is_JavaThread_protected_by_my_ThreadsList(const
>>> JavaThread* p) {
>>> 497: Thread*
On 16/10/2021 8:26 am, Daniel D.Daugherty wrote:
On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes 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
On Fri, 15 Oct 2021 22:19:15 GMT, Daniel D. Daugherty
wrote:
>> So suspend_thread and resume thread's caller already takes a
>> ThreadsListHandle so this is unnecessary and never happens.
>
>> This seems an unrelated change in behaviour ??
>
> Actually this is equivalent code. The baseline
On Fri, 15 Oct 2021 21:58:38 GMT, Daniel D. Daugherty
wrote:
>> src/hotspot/share/runtime/handshake.cpp line 358:
>>
>>> 356: bool target_is_dead = false;
>>> 357: if (target == nullptr) {
>>> 358: target_is_dead = true;
>>
>> Why would you pass a NULL target thread to
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
On Fri, 15 Oct 2021 06:54:10 GMT, David Holmes wrote:
> 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
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
On Fri, 15 Oct 2021 18:27:43 GMT, Coleen Phillimore wrote:
>> src/hotspot/share/runtime/thread.cpp line 1771:
>>
>>> 1769: guarantee(Thread::is_JavaThread_protected_by_my_ThreadsList(this),
>>> 1770: "missing ThreadsListHandle in calling context.");
>>> 1771: if (is_exiting()) {
On Fri, 15 Oct 2021 18:20:12 GMT, Coleen Phillimore wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/runtime/handshake.cpp line 358:
>
>> 356: bool target_is_dead =
On Fri, 15 Oct 2021 06:34:42 GMT, David Holmes 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
On Fri, 15 Oct 2021 06:45:02 GMT, David Holmes wrote:
>> Daniel D. Daugherty has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> 8249004.cr1.patch
>
> src/hotspot/share/runtime/thread.cpp line 1771:
>
>> 1769:
On Thu, 14 Oct 2021 16:03:28 GMT, Daniel D. Daugherty
wrote:
>> 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.
>
> Daniel
On Thu, 14 Oct 2021 16:03:28 GMT, Daniel D. Daugherty
wrote:
>> 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.
>
> Daniel
On Sun, 4 Jul 2021 23:39:00 GMT, David Holmes wrote:
>> 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.
>
> Hi Dan,
>
> I
> 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.
Daniel D. Daugherty has updated the pull request incrementally with one
57 matches
Mail list logo
