RFR(XS): 8224573: Fix windows build after JDK-8221507

Wed May 22 06:59:12 UTC 2019

Looks okay.

32bit should be enough for number of regions I think. If I am not mistaken,
they have a min region size of 256K, so this covers a heap of ~1000 tb?

..Thomas

On Wed, May 22, 2019 at 8:47 AM Langer, Christoph <christoph.langer at sap.com>
wrote:

> Hi,
>
> please review this build fix for Windows:
>
> Bug: https://bugs.openjdk.java.net/browse/JDK-8224573
> Webrev: http://cr.openjdk.java.net/~clanger/webrevs/8224573.0/
>
> Thanks
> Christoph
>
>
>
> > -----Original Message-----
> > From: hotspot-jfr-dev <hotspot-jfr-dev-bounces at openjdk.java.net> On
> > Behalf Of Langer, Christoph
> > Sent: Dienstag, 21. Mai 2019 22:02
> > To: Ken Dobson <kdobson at redhat.com>
> > Cc: hotspot-jfr-dev at openjdk.java.net
> > Subject: [CAUTION] Build error on Windows after 8221507: Implement JFR
> > Events for Shenandoah
> >
> > 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
> > > >> >>
> > > >> >
> > > >>
> > > >>
> > > >
>