Shenandoah Events

[zgu at redhat.com]

On Mon, 2019-01-21 at 12:32 +0100, Aleksey Shipilev wrote:
> On 1/18/19 10:02 PM, Ken Dobson wrote:
> > Actually I've just gotten my authorship so here is a link to the
> > webrev.
> > http://cr.openjdk.java.net/~kdobson/serializer/webrev/
> 
> Aha. How did you test it? Because the quick test fails with lots of
> asserts:
>  $ CONF=linux-x86_64-server-fastdebug make images run-test
> TEST=tier1_gc_shenandoah
> 
> The patch is better to pass more complete test:
>  $ CONF=linux-x86_64-server-fastdebug make images run-test
> TEST=hotspot_gc_shenandoah

Also, verify builds with --with-jvm-features=-jfr, make sure they don't
break.

Thanks,

-Zhengyu


> 
> Other comments:
> 
>  *) Copyright years need to be updated.
> 
>  *) You need to register serializers once, not for each region?
> 
>  *) It makes sense to put jfr_register_serializers into Shenandoah
> namespace
> 
>  *) There is no need for separate method for JFR event commit, the
> whole point of set_state was to
> accept the JFR event inside
> 
>  *) No need for "// INCLUDE_SHENANDOAHGC" when if is short.
> 
>  *) Code style: "if(evt.should_commit())" should have space after
> "if".
> 
>  *) Do not change methods visibility freely. If you want to hack into
> private members of
> ShenandoahHeapRegion, befriend the classes.
> 
> You can fix it yourself (good exercise anyway), or apply this patch
> on top:
>  http://cr.openjdk.java.net/~shade/shenandoah/jfr-review-1.patch
> 
> -Aleksey