Hi all,
here comes round 4 of Shenandoah upstreaming review:
This includes fixes for the issues that Per brought up:
- Verify and gracefully reject dangerous flags combinations that
disables required barriers
- Revisited @requires filters in tests
- Trim unused code from Shenandoah's SA impl
- Mo
Hi, I looked at the runtime changes, which are very few, since you've
upstreamed most of the runtime dependent changes already.
They look good to me.
Thanks,
Coleen
(trying again with all the lists).
On 11/30/18 4:00 PM, Roman Kennke wrote:
Hi all,
here comes round 4 of Shenandoah upstreaming
Much better.
Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp ? Why not include only shenandoahSupport.hpp
when new nodes are defined?
I think it is fine to not use #ifdef in loopopts.cpp when you check is_ShenandoahBarrier(). And you don't do that in
other files.
Cod
Hi Vladimir,
> Much better.
Thanks!
> Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp
> ? Why not include only shenandoahSupport.hpp when new nodes are defined?
For no particular reason, I guess. I'll fix it in next round.
> I think it is fine to not use #ifdef in loopo
Hello Vladimir,
> Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp
> ? Why not include only shenandoahSupport.hpp when new nodes are defined?
I discussed this with my team. shenandoahBarrierSetC2.hpp is supposed to
be the entry point for external C2 code. No C2 code is supp
On 12/2/18 8:55 AM, Roman Kennke wrote:
Hello Vladimir,
Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp
? Why not include only shenandoahSupport.hpp when new nodes are defined?
I discussed this with my team. shenandoahBarrierSetC2.hpp is supposed to
be the entry point
>> Hello Vladimir,
>>
>>> Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp
>>> ? Why not include only shenandoahSupport.hpp when new nodes are defined?
>>
>> I discussed this with my team. shenandoahBarrierSetC2.hpp is supposed to
>> be the entry point for external C2 code. N
Yes, compiler changes reviewed.
Vladimir
On 12/2/18 10:15 AM, Roman Kennke wrote:
Hello Vladimir,
Why you need to include shenandoahBarrierSetC2.hpp in /opto/classes.cpp
? Why not include only shenandoahSupport.hpp when new nodes are defined?
I discussed this with my team. shenandoahBarrier
On Nov 30, 2018, at 3:22 PM, coleen.phillim...@oracle.com wrote:
>
> Hi, I looked at the runtime changes, which are very few, since you've
> upstreamed most of the runtime dependent changes already.
> They look good to me.
"since you've upstreamed most of the runtime dependent changes already"
Hi Roman,
A few comments on the SA changes:
==> Could you please add the following lines in
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/HSDB.java from line
1120 onwards to avoid the "[Unknown generation]" message with hsdb while
displaying the Stack Memory for a mutator thread ?
els
Hi Jini,
Thanks for your suggestions. I've added this to Shenandoah's dev:
http://cr.openjdk.java.net/~rkennke/shenandoah-sa/webrev.01/
and it will show up in next round of webrevs.
Thanks,
Roman
> A few comments on the SA changes:
>
> ==> Could you please add the following lines in
> src/jd
Hi Roman,
On 11/30/18 10:00 PM, Roman Kennke wrote:
Hi all,
here comes round 4 of Shenandoah upstreaming review:
This includes fixes for the issues that Per brought up:
- Verify and gracefully reject dangerous flags combinations that
disables required barriers
- Revisited @requires filters in
Hi Per,
Thanks for looking again.
I've fixed the ordering in shenandoah-dev:
http://cr.openjdk.java.net/~rkennke/fix-macro-order/webrev.00/
and it will apear in the next round of webrevs.
Thanks,
Roman
> Hi Roman,
>
> On 11/30/18 10:00 PM, Roman Kennke wrote:
>> Hi all,
>>
>> here comes rou
Round 5 of Shenandoah review includes:
- A fix for the @requires tag in TestFullGCCountTest.java. It should be
correct now. We believe the CMS @requires was also not quite right and
fixed it the same.
It reads now: Don't run this test if:
- Actual GC set by harness is CMS *and* ExplicitGCInvokesC
> 3 dec. 2018 kl. 20:27 skrev Roman Kennke :
>
> Round 5 of Shenandoah review includes:
> - A fix for the @requires tag in TestFullGCCountTest.java. It should be
> correct now. We believe the CMS @requires was also not quite right and
> fixed it the same.
>
> It reads now: Don't run this test i
Hi Roman,
Thank you for making the changes. The SA portion looks good to me. One
nit though:
In
src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/HeapSummary.java,
in printGCAlgorithm(), does displaying the nbr of Parallel GC threads
not make sense for Shenandoah (like it is for G1,
Hi Magnus,
>> I got reviews for shared-build, but an earlier version, so maybe makes
>> sense to look over this again. Erik J, Magnus?
>
> Build changes look good.
Thanks, Magnus!
Roman
signature.asc
Description: OpenPGP digital signature
Hi Jini,
> Thank you for making the changes. The SA portion looks good to me.
Thank you!
> One
> nit though:
>
> In
> src/jdk.hotspot.agent/share/classes/sun/jvm/hotspot/tools/HeapSummary.java,
> in printGCAlgorithm(), does displaying the nbr of Parallel GC threads
> not make sense for Shenando
Hi Roman,
On 12/3/18 8:27 PM, Roman Kennke wrote:
Round 5 of Shenandoah review includes:
- A fix for the @requires tag in TestFullGCCountTest.java. It should be
correct now. We believe the CMS @requires was also not quite right and
fixed it the same.
It reads now: Don't run this test if:
- Ac
Hi Per,
>> Round 5 of Shenandoah review includes:
>> - A fix for the @requires tag in TestFullGCCountTest.java. It should be
>> correct now. We believe the CMS @requires was also not quite right and
>> fixed it the same.
>>
>> It reads now: Don't run this test if:
>> - Actual GC set by harness i
20 matches
Mail list logo
