RFR: Bulk backports sh/jdk11 -> sh/jdk8u

Roman Kennke rkennke at redhat.com
Sat Mar 16 10:36:35 UTC 2019


Hi Aleksey,

Thanks for reviewing! Some comments below:

> On 3/15/19 10:48 PM, Roman Kennke wrote:
>> This backports outstanding changes from mid-Dec to ~now from shenandoah/jdk11 to shenandoah/jdk8u.
>>
>> Find the list of changes here:
>> http://cr.openjdk.java.net/~rkennke/backports-sh-jdk8-2019-03-15/changes.txt
>>
>> The full webrev:
>> http://cr.openjdk.java.net/~rkennke/backports-sh-jdk8-2019-03-15/webrev.00/
> 
> *) This comment seem superfluous:
> 
>    57   if (os::create_thread(this, os::cgc_thread)) {
>    58     // XXX: need to set this to low priority
>    59     // unless "agressive mode" set; priority
>    60     // should be just less than that of VMThread.
>    61     os::set_priority(this, ShenandoahCriticalControlThreadPriority ? CriticalPriority :
> NearMaxPriority);
>    62     if (!_should_terminate && !DisableStartThread) {
>    63       os::start_thread(this);
>    64     }
>    65   }

I removed this.

> *) Indenting is weird here:
> 
>    58 template <class E, MEMFLAGS F, unsigned int N>
>    59 void BufferedOverflowTaskQueue<E, F, N>::clear() {
>    60     _buf_empty = true;
>    61     taskqueue_t::set_empty();
>    62     taskqueue_t::overflow_stack()->clear();
>    63 }

Yeah. It's also present in newer code. This warrants fwd-porting.

> *) Do we have "commonming" typo in other versions?
> 
>   391   experimental(bool, ShenandoahCommonGCStateLoads, false,                   \
>   392          "Enable commonming for GC state loads in generated code.")         \

Yes, indeed. This also should be fwd ported.

> *) Asserts seem excessive here, already under UseShenandoahGC branch:
> 
> 2349 #if INCLUDE_ALL_GCS
> 2350   if (UseShenandoahGC) {
> 2351     if (mode == LoopOptsShenandoahExpand) {
> 2352       assert(UseShenandoahGC, "only for shenandoah");
> 2353       ShenandoahWriteBarrierNode::pin_and_expand(this);
> 2354     } else if (mode == LoopOptsShenandoahPostExpand) {
> 2355       assert(UseShenandoahGC, "only for shenandoah");
> 2356       visited.Clear();
> 2357       ShenandoahWriteBarrierNode::optimize_after_expansion(visited, nstack, worklist, this);
> 2358     }

Newer versions don't have 'UseShenandoahGC' around the part, but should 
only ever executed (as asserted). I suggest we either remove the if 
(UseShenandoahGC) in 8, or add it to later versions. Or maybe just 
remove the assert in 8 and live with the difference? What do you think?

> Otherwise looks good. I haven't reviewed C2 parts carefully.

Updated webrev (haven't updated the C2 part):
http://cr.openjdk.java.net/~rkennke/backports-sh-jdk8-2019-03-15/webrev.01/

Roman


More information about the shenandoah-dev mailing list