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

Wed May 22 15:25:36 UTC 2019

Hi Ken, I believe this is already fixed, see 8224573.

Regards, Thomas

On Wed, May 22, 2019 at 5:11 PM Ken Dobson <kdobson at redhat.com> wrote:

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