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

Wed May 22 05:00:11 UTC 2019

I am confused how this can even happen, should jdk-submit tests not have
captured that before pushing?

Cheers, Thomas

On Tue, May 21, 2019 at 10:02 PM Langer, Christoph <christoph.langer at sap.com>
wrote:

> 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
> > >> >>
> > >> >
> > >>
> > >>
> > >
>