[11u] RFR: 8222518: Remove unnecessary caching of Parker object in java.lang.Thread
Hohensee, Paul
hohensee at amazon.com
Fri Jan 15 19:33:48 UTC 2021
Thanks, Richard, I agree. :)
Paul
-----Original Message-----
From: "Reingruber, Richard" <richard.reingruber at sap.com>
Date: Thursday, January 14, 2021 at 11:37 PM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: Andrew Haley <aph at redhat.com>, Volker Simonis <volker.simonis at gmail.com>, "jdk-updates-dev at openjdk.java.net" <jdk-updates-dev at openjdk.java.net>
Subject: RE: [11u] RFR: 8222518: Remove unnecessary caching of Parker object in java.lang.Thread
Hi Paul,
AFAICT your change should not have any performance effect besides the reduced
footprint. Performance wise it should not matter whether you get the Parker* (A)
from the j.l.Thread instance or (B) from the native JavaThread. Wouldn't you
agree?
On path (B) Threads_lock was acquired before SMR was introduced. Since SMR was
introduced[1] a ThreadsListHandle ist acquired instead of Threads_lock in both
cases (A) and (B). Actually I don't think this is necessary for (A) and I reckon
that's the fastpath David Holmes intended to reintroduce depending on his
performance tests.
So the performance test should compare the current version w/o your change
Current Version
1 UNSAFE_ENTRY(void, Unsafe_Unpark(JNIEnv *env, jobject unsafe, jobject jthread)) {
2 Parker* p = NULL;
3
4 if (jthread != NULL) {
5 ThreadsListHandle tlh;
6 JavaThread* thr = NULL;
7 oop java_thread = NULL;
8 (void) tlh.cv_internal_thread_to_JavaThread(jthread, &thr, &java_thread);
9 if (java_thread != NULL) {
10 // This is a valid oop.
11 jlong lp = java_lang_Thread::park_event(java_thread);
12 if (lp != 0) {
13 // This cast is OK even though the jlong might have been read
14 // non-atomically on 32bit systems, since there, one word will
15 // always be zero anyway and the value set is always the same
16 p = (Parker*)addr_from_java(lp);
17 } else {
18 // Not cached in the java.lang.Thread oop yet (could be an
19 // older version of library).
20 if (thr != NULL) {
21 // The JavaThread is alive.
22 p = thr->parker();
23 if (p != NULL) {
24 // Cache the Parker in the java.lang.Thread oop for next time.
25 java_lang_Thread::set_park_event(java_thread, addr_to_java(p));
26 }
27 }
28 }
29 }
30 } // ThreadsListHandle is destroyed here.
31
32 if (p != NULL) {
33 HOTSPOT_THREAD_UNPARK((uintptr_t) p);
34 p->unpark();
35 }
36 } UNSAFE_END
VERSUS something like
Optimized Version
38 UNSAFE_ENTRY(void, Unsafe_Unpark(JNIEnv *env, jobject unsafe, jobject jthread)) {
39 Parker* p = NULL;
40
41 if (jthread != NULL) {
42 oop java_thread = JNIHandles::resolve_non_null(jthread);
43 // This is a valid oop.
44 jlong lp = java_lang_Thread::park_event(java_thread);
45 if (lp != 0) {
46 // This cast is OK even though the jlong might have been read
47 // non-atomically on 32bit systems, since there, one word will
48 // always be zero anyway and the value set is always the same
49 p = (Parker*)addr_from_java(lp);
50 } else {
51 ThreadsListHandle tlh;
52 JavaThread* thr = NULL;
53 oop java_thread = NULL;
54 (void) tlh.cv_internal_thread_to_JavaThread(jthread, &thr, &java_thread);
55 // Not cached in the java.lang.Thread oop yet (could be an
56 // older version of library).
57 if (thr != NULL) {
58 // The JavaThread is alive.
59 p = thr->parker();
60 if (p != NULL) {
61 // Cache the Parker in the java.lang.Thread oop for next time.
62 java_lang_Thread::set_park_event(java_thread, addr_to_java(p));
63 }
64 }
65 } // ThreadsListHandle is destroyed here.
66 }
67
68 if (p != NULL) {
69 HOTSPOT_THREAD_UNPARK((uintptr_t) p);
70 p->unpark();
71 }
72 } UNSAFE_END
Line 49 would be similar to the old fast path before SMR.
Line 51 would be the slow path where a ThreadsListHandle is acquired.
This would be my understanding of the performance question.
Your change does look good to me. It reduces footprint without performance cost.
Personally I think it can be downported as clean-up. Just my .02€.
Cheers, Richard.
[1] https://github.com/openjdk/jdk/commit/0dff96ff0bf4a1331b3b012933266284d6c2e917#
-----Original Message-----
From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> On Behalf Of Hohensee, Paul
Sent: Mittwoch, 13. Januar 2021 20:19
To: Volker Simonis <volker.simonis at gmail.com>
Cc: Andrew Haley <aph at redhat.com>; jdk-updates-dev at openjdk.java.net
Subject: RE: [11u] RFR: 8222518: Remove unnecessary caching of Parker object in java.lang.Thread
Thanks once more, Volker. :)
An m4.10xlarge is dual socket and the runs I posted were done running on both. I've just added a comparison running on a single socket. It shows much less variance, small gains on three of the sub-workloads, and a minor (< 1%) loss on the fourth.
Thanks,
Paul
-----Original Message-----
From: Volker Simonis <volker.simonis at gmail.com>
Date: Wednesday, January 13, 2021 at 10:47 AM
To: "Hohensee, Paul" <hohensee at amazon.com>
Cc: Andrew Haley <aph at redhat.com>, "jdk-updates-dev at openjdk.java.net" <jdk-updates-dev at openjdk.java.net>
Subject: RE: [11u] RFR: 8222518: Remove unnecessary caching of Parker object in java.lang.Thread
Thanks Paul.
Still looks good to me :)
On Wed, Jan 13, 2021 at 5:32 PM Hohensee, Paul <hohensee at amazon.com> wrote:
>
> I've added performance data to the JBS issue (https://bugs.openjdk.java.net/browse/JDK-8222518).
>
> Thanks,
> Paul
>
> -----Original Message-----
> From: jdk-updates-dev <jdk-updates-dev-retn at openjdk.java.net> on behalf of Andrew Haley <aph at redhat.com>
> Date: Tuesday, January 12, 2021 at 2:36 AM
> To: "jdk-updates-dev at openjdk.java.net" <jdk-updates-dev at openjdk.java.net>
> Subject: RE: [11u] RFR: 8222518: Remove unnecessary caching of Parker object in java.lang.Thread
>
> On 12/23/20 4:13 PM, Hohensee, Paul wrote:
> > I've little knowledge or experience in the area of hotspot thread synchronization. With that disclaimer, the patch looks benign to me but I would want to hear another expert's assessment before being able to approve this with a good conscience.
> >
> > I could ask Richard from our team (on cc) to have a look when he's back from vacation. Or feel free to find somebody else to review and assess this backport from a technical pov.
>
> This area does seem to ave changed a fair bit. Do we know if the
> performance analysis done by David Holmes in 2019 is valid for JDK 11?
> We should check that it is before applying this.
>
> --
> Andrew Haley (he/him)
> Java Platform Lead Engineer
> Red Hat UK Ltd. <https://www.redhat.com>
> https://keybase.io/andrewhaley
> EAC8 43EB D3EF DB98 CC77 2FAD A5CD 6035 332F A671
>
>
More information about the jdk-updates-dev
mailing list