[11u] RFR: 8222518: Remove unnecessary caching of Parker object in java.lang.Thread

Fri Jan 15 07:37:20 UTC 2021

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