Build error on Windows after 8221507: Implement JFR Events for Shenandoah
Ken Dobson
kdobson at redhat.com
Wed May 22 15:44:16 UTC 2019
Well that's easy then, thanks Christoph.
Ken Dobson
On Wed, May 22, 2019 at 11:26 AM Thomas Stüfe <thomas.stuefe at gmail.com>
wrote:
> 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
>>>> > >> >>
>>>> > >> >
>>>> > >>
>>>> > >>
>>>> > >
>>>>
>>>>
More information about the hotspot-jfr-dev
mailing list