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

Wed May 22 15:11:36 UTC 2019

Just looking into this now but it appears it's a casting issue and this is
unlikely to be an issue unique to Windows. I'll open a bug and post a fix
after I've verified.

Ken Dobson

On Wed, May 22, 2019 at 3:47 AM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:

> Ah, that explains it. Since I rely on jdk-submit to catch my errors too, I
> got nervous for a moment.
>
> On Wed, May 22, 2019 at 9:44 AM Langer, Christoph <
> christoph.langer at sap.com> wrote:
>
>> Hi Thomas,
>>
>>
>>
>> I believe it was not caught because Oracle/jdk-submit builds with
>> Shenandoah turned off…
>>
>>
>>
>> /Christoph
>>
>>
>>
>> *From:* Thomas Stüfe <thomas.stuefe at gmail.com>
>> *Sent:* Mittwoch, 22. Mai 2019 07:00
>> *To:* Langer, Christoph <christoph.langer at sap.com>
>> *Cc:* Ken Dobson <kdobson at redhat.com>; hotspot-jfr-dev at openjdk.java.net
>> *Subject:* Re: Build error on Windows after 8221507: Implement JFR
>> Events for Shenandoah
>>
>>
>>
>> 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
>> > >> >>
>> > >> >
>> > >>
>> > >>
>> > >
>>
>>