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