Build error on Windows after 8221507: Implement JFR Events for Shenandoah

Tue May 21 20:01:46 UTC 2019

Hi Ken,

after you pushed JDK-8221507, I see build errors on Windows (enabled warnings as errors):

shenandoahHeapRegion.cpp
.../jdk/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp(687): error C2220: warning treated as error - no 'object' file generated
.../jdk/src/hotspot/share/gc/shenandoah/shenandoahHeapRegion.cpp(687): warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible loss of data
   ... (rest of output omitted)
* For target hotspot_variant-server_libjvm_objs_shenandoahJfrSupport.obj:
shenandoahJfrSupport.cpp
.../jdk/src/hotspot/share/gc/shenandoah/shenandoahJfrSupport.cpp(60): error C2220: warning treated as error - no 'object' file generated
.../jdk/src/hotspot/share/gc/shenandoah/shenandoahJfrSupport.cpp(60): warning C4267: 'argument': conversion from 'size_t' to 'unsigned int', possible loss of data
   ... (rest of output omitted)

I don't know if the fix is to simply add a cast to unsigned int. Can you please check and repair?

Thanks
Christoph

> -----Original Message-----
> From: jmc-dev <jmc-dev-bounces at openjdk.java.net> On Behalf Of Ken
> Dobson
> Sent: Dienstag, 14. Mai 2019 23:19
> To: Erik Gahlin <erik.gahlin at oracle.com>
> Cc: hotspot-jfr-dev at openjdk.java.net; mikhailo.seledtsov at oracle.com; jmc-
> dev at openjdk.java.net
> Subject: Re: RFR 8221507: Implement JFR Events for Shenandoah
> 
> Thank you for the review again.
> 
> Yes, not sure regarding backporting plans but I imagine it will be in the
> future so seemed best to avoid anything that cause issues such as Set.of().
> 
> I also think the naming of the jfr's is fine as it's stored in a folder
> named after the test which was easily found now that I'm aware that the
> jfrs are already dumped.
> 
> I believe I've addressed the rest of your recommended changes to the tests,
> let me know if I've covered everything.
> 
> http://cr.openjdk.java.net/~kdobson/eventswithtests/webrev/
> 
> Thanks,
> 
> Ken Dobson
> 
> On Sun, May 12, 2019 at 8:28 PM Erik Gahlin <erik.gahlin at oracle.com> wrote:
> 
> > Thanks for fixing!
> >
> > Product code looks good, but I think tests could be improved.
> >
> > There is no need to call dump in a final clause.
> >
> > Events.fromRecording(...) will dump the file into the jtreg scratch
> > directory ( "recording-1-pid-xxx.jfr")  by default. I think this is
> > sufficient since there is only one recording per test.  If you think the
> > name is cryptic, consider adding another method to Event.fromRecording
> that
> > takes an additional parameter that becomes a suffix, i.e
> > "recording-1-pid-xxx-test-heap-region-info.jfr"
> >
> > try (Recording r = new recording()) {
> >   r.enable(EVENT_NAME);
> >   r.start();
> >   r.stop()
> >   List<RecordedEvent> events = Events.fromRecording(r);
> >   Events.hasEvents(events);
> >   for (RecordedEvent event : events) {
> >     Events.assertField(event, "index").notEqual(-1);
> >     Events.assertField(event, "used").atMost(1024*1024L);
> >     String state = Events.assertField(event, "state").getValue();
> >     GCHelper.assertShenandoahHeapRegionState(state));
> > }
> >
> > The name of the field in metadata.xml is "state", but the tests looks for
> > "type". That seems incorrect.
> >
> > GCHelper:
> >
> > public static assertShenandoahHeapRegionState(String state) {
> >  if (!shenandoahHeapRegionStates.contains(state)) {
> >     throw new AssertionError("Unknown state '"+ state +"', valid heap
> > region states are " + shenandoahHeapRegionStates);
> > }
> >
> > I assume you don't use Set.of("Empty Uncommitted" , ... , "Trash")
> > because you want to backport to JDK 8.
> >
> > Thanks
> > Erik
> >
> > Thanks everyone for their reviews. I've added tests similar to the tests
> > for the G1 events as well as addressed the changes that Erik
> recommended.
> >
> > Here is the new webrev:
> >
> http://cr.openjdk.java.net/~kdobson/shenandoaheventswithtests/webrev/
> >
> > Please let me know if there's anything else that should be addressed.
> >
> > Thanks,
> >
> > Ken Dobson
> >
> > On Mon, May 6, 2019 at 11:38 AM <mikhailo.seledtsov at oracle.com>
> wrote:
> >
> >>
> >>
> >> On 5/4/19 2:16 PM, Erik Gahlin wrote:
> >> > On 2019-05-04 00:19, mikhailo.seledtsov at oracle.com wrote:
> >> >> Hi,
> >> >>
> >> >>   If possible and feasible, could you please implement tests for new
> >> >> events?
> >> >>
> >> >> Please place the tests under test/jdk/jdk/jfr/event/gc/. Perhaps
> >> >> create a "shenandoah" subfolder.
> >> >>
> >> >> If the events are too hard to test or not feasible, please add the
> >> >> event names to
> >> >> test/jdk/jdk/jfr/event/metadata/TestLookForUntestedEvents.java, to
> >> >> the hardToTestEvents set. Consider commenting why it is too hard to
> >> >> test.
> >> >
> >> > If events are are marked experimental, tests are ignored by
> >> > TestLookForUntestedEvents. java, so we should not  add them to the
> >> > hardToTestEvents set. That may help them pass under the radar once
> the
> >> > experimental attribute is removed.
> >> Thank you. If the events are "experimental" this approach makes sense.
> >> Once the experimental status is removed, the
> >> TestLookForUntestedEvents.java should be able to notice if the event is
> >> not covered by tests.
> >> > That said, tests never hurts, even if they are experimental. I think
> >> > test/jdk/jdk/jfr/event/gc/detailed would be a good place
> >> +1
> >>
> >> Thank you,
> >> Misha
> >> >
> >> > Erik
> >> >
> >> >>
> >> >> Also, please make sure to run all jfr tests under test/jdk/jdk/jfr/
> >> >> prior to integration.
> >> >>
> >> >>
> >> >> Best regards,
> >> >>
> >> >> Misha
> >> >>
> >> >>
> >> >>
> >> >> On 5/3/19 7:44 AM, Ken Dobson wrote:
> >> >>> Hi all,
> >> >>>
> >> >>> Please review this patch that adds support for two new JFR events
> >> >>> ShenandoahHeapRegionStateChange and
> ShenandoahHeapRegionInformation.
> >> >>>
> >> >>> Bug: https://bugs.openjdk.java.net/browse/JDK-8221507
> >> >>> Webrev: http://cr.openjdk.java.net/~kdobson/53476/webrev/
> >> >>>
> >> >>> The Shenandoah team has also reviewed this patch and approved it
> >> >>> from their
> >> >>> end.
> >> >>>
> >> >>> Thanks,
> >> >>>
> >> >>> Ken Dobson
> >> >>
> >> >
> >>
> >>
> >