PING: RFR: 8220657: JFR.dump does not work when filename is set
Yasumasa Suenaga
yasuenag at gmail.com
Thu Jun 13 07:29:38 UTC 2019
Looks good!
Thanks,
Yasumasa
2019年6月13日(木) 16:26 Erik Gahlin <erik.gahlin at oracle.com>:
> Hi Yasumasa,
>
> Here is a fix that I think will work.
>
> http://cr.openjdk.java.net/~egahlin/8220657%EF%80%BA/
>
> Thanks
> Erik
>
> > PING:
> >
> > Could you review this webrev?
> >
> > > http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.03/
> >
> >
> > Yasumasa
> >
> >
> > On 2019/05/03 23:05, Yasumasa Suenaga wrote:
> >> Hi Erik,
> >>
> >> I uploaded new webrev:
> >> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.03/
> >>
> >> This webrev does not use `var` in any place.
> >> And I tweaked testcase based on your idea.
> >>
> >>
> >> Thanks,
> >>
> >> Yasumasa
> >>
> >>
> >> On 2019/05/02 23:41, Erik Gahlin wrote:
> >>> Hi Yasumasa,
> >>>
> >>> The test looks complicated.
> >>>
> >>> I would prefer something more straight forward with less states and
> >>> nulls where the expected behavior is easy to see. If the test fails,
> >>> it's also nice to see from the stack trace which sub test failed.
> >>>
> >>> public static void main(String... args) throws Exception {
> >>> testDumpAll();
> >>> testDumpNamed();
> >>> testDumpNamedWithFilename();
> >>> }
> >>>
> >>> private static void testDumpAll() throws Exception {
> >>> Path p = Path.of("A.jfr").toAbsolutePath();
> >>> try (Recording r = new Recording()) {
> >>> r.setName("A");
> >>> r.setDestination(p);
> >>> JcmdHelper.jcmd("JFR.dump");
> >>> Asserts.assertFalse(namedFile(p), "Unexpected file: " + p);
> >>> Asserts.assertTrue(generatedFile(), "Expected generated file");
> >>> }
> >>> }
> >>>
> >>> private static void testDumpNamed() throws Exception {
> >>> Path p = Path.of("B.jfr").toAbsolutePath();
> >>> try (Recording r = new Recording()) {
> >>> r.setName("B");
> >>> r.setDestination(p);
> >>> JcmdHelper.jcmd("JFR.dump", "name=B");
> >>> Asserts.assertTrue(namedFile(p), "Expected file: " + p);
> >>> Asserts.assertFalse(generatedFile(), "Unexpected generated
> >>> file");
> >>> }
> >>> }
> >>>
> >>> private static void testDumpNamedWithFilename() throws Exception {
> >>> Path p = Path.of("C.jfr").toAbsolutePath();
> >>> Path override = Path.of("override.jfr").toAbsolutePath();
> >>> try (Recording r = new Recording()) {
> >>> r.setName("C");
> >>> r.setDestination(p);
> >>> JcmdHelper.jcmd("JFR.dump", "name=C", "filename=" + override);
> >>> Asserts.assertFalse(namedFile(p), "Unexpected file: " + p);
> >>> Asserts.assertTrue(namedFile(p), "Expected file: " + override);
> >>> Asserts.assertFalse(generatedFile(), "Unexpected generated
> >>> file");
> >>> }
> >>> }
> >>>
> >>> private static boolean namedFile(Path dumpFile) throws IOException {
> >>> return Files.exists(dumpFile) && Files.size(dumpFile) > 0;
> >>> }
> >>>
> >>> private static boolean generatedFile() throws IOException {
> >>> long pid = ProcessHandle.current().pid();
> >>> return Files.find(Path.of("."), 1, (p, a) -> p.toString()
> >>> .matches("^.*hotspot-pid-" + pid + ".jfr$") &&
> >>> a.size() > 0L)
> >>> .findFirst().isPresent();
> >>> }
> >>>
> >>> I haven't tried it.
> >>>
> >>> When it comes to DCmdDump, I would prefer if you would not use "var"
> >>> as this hides the underlying type (SafePath or WriteableUserPath)
> >>> which is used to signal if there is a security risk or not. I think
> >>> it is fine, but if the code gets refactor at a later stage the
> >>> information could get lost.
> >>>
> >>> Thanks
> >>> Erik
> >>>
> >>>> PING: Could you review it?
> >>>>
> >>>> > http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.02/
> >>>>
> >>>>
> >>>> Yasumasa
> >>>>
> >>>>
> >>>> On 2019/04/24 21:43, Yasumasa Suenaga wrote:
> >>>>> Hi Erik,
> >>>>>
> >>>>> I uploaded new webrev:
> >>>>>
> >>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.02/
> >>>>>
> >>>>> I kept to remove setDestination() call in PlatformRecording.
> >>>>> In this webrev, original filename value will be set to temporary
> >>>>> snapshot if DCmdDump does not have recording name and filename.
> >>>>>
> >>>>> Also I added new testcase. It can test JFR.dump with filename.
> >>>>>
> >>>>>
> >>>>> Thanks,
> >>>>>
> >>>>> Yasumasa
> >>>>>
> >>>>>
> >>>>> On 2019/04/23 6:48, Erik Gahlin wrote:
> >>>>>> I have looked further at the issue and while the message to the
> >>>>>> user was incorrect previously ( "Dump failed. No data found in
> >>>>>> the specified interval") it did result in a file with the
> >>>>>> specified filename, i.e "test.jfr"
> >>>>>>
> >>>>>> When "clone.setDestination(this.destination);" is removed, the
> >>>>>> filename passed on command line is ignored, but it does result in
> >>>>>> a message that is consistent with what is happening (a file with
> >>>>>> an autogenerated name is created). It does however change the
> >>>>>> behavior of 'JFR.dump <pid> name=1'. Logic needs to be added to
> >>>>>> DCmdDump so it uses an existing filename if it is set, but
> >>>>>> changes it if a user is providing an override.
> >>>>>>
> >>>>>> When comes to testing. I think it would be better to write a
> >>>>>> separate test that checks that things are dumped correctly
> >>>>>> when-XX:StartFlightRecording:filename=<filename> is used.
> >>>>>>
> >>>>>> Thanks
> >>>>>> Erik
> >>>>>>
> >>>>>>> PING: Do you have any opinion(s) about this fix?
> >>>>>>>
> >>>>>>>
> >>>>>>> Yasumasa
> >>>>>>>
> >>>>>>>
> >>>>>>> On 2019/04/01 21:46, Yasumasa Suenaga wrote:
> >>>>>>>> Hi,
> >>>>>>>>
> >>>>>>>> Do you have concern(s) about this change?
> >>>>>>>>
> >>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.01/
> >>>>>>>>
> >>>>>>>> As in the email below, I think it does not affect scenarios
> >>>>>>>> except snapshot cloning.
> >>>>>>>> Please tell me if you have any opinions about this.
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> Thanks,
> >>>>>>>>
> >>>>>>>> Yasumasa
> >>>>>>>>
> >>>>>>>>
> >>>>>>>> On 2019/03/20 10:37, Yasumasa Suenaga wrote:
> >>>>>>>>> Hi Erik,
> >>>>>>>>>
> >>>>>>>>> I think this change does not affect other scenarios.
> >>>>>>>>>
> >>>>>>>>> :jdk_jfr jtreg tests and submit repo have passed with this
> >>>>>>>>> change.
> >>>>>>>>> newSnapshotClone() is called by PlatformRecording::dump and
> >>>>>>>>> DCmdDump::newSnapshot.
> >>>>>>>>> Both methods are related to user-requested dump.
> >>>>>>>>>
> >>>>>>>>> What scenarios do you think? I want to check them.
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> Thanks,
> >>>>>>>>>
> >>>>>>>>> Yasumasa
> >>>>>>>>>
> >>>>>>>>>
> >>>>>>>>> 2019年3月19日(火) 23:59 Erik Gahlin <erik.gahlin at oracle.com>:
> >>>>>>>>>>
> >>>>>>>>>> Yes, but then you would lose the original destination in
> >>>>>>>>>> other scenarios
> >>>>>>>>>>
> >>>>>>>>>> Maybe I can look at this next week.
> >>>>>>>>>>
> >>>>>>>>>> Erik
> >>>>>>>>>>
> >>>>>>>>>>> Hi Erik,
> >>>>>>>>>>>
> >>>>>>>>>>> Thank you for your reply.
> >>>>>>>>>>>
> >>>>>>>>>>> How about this changeset?
> >>>>>>>>>>>
> >>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.01/
> >>>>>>>>>>>
> >>>>>>>>>>> I think PlatformRecording#newSnapshotClone() should not dump.
> >>>>>>>>>>> So I removed `setDestination()` from this.
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> Yasumasa
> >>>>>>>>>>>
> >>>>>>>>>>>
> >>>>>>>>>>> On 2019/03/19 22:04, Erik Gahlin wrote:
> >>>>>>>>>>>> This looks a bit hackish.
> >>>>>>>>>>>>
> >>>>>>>>>>>> The real issue seems to be that the method
> >>>>>>>>>>>> PlatformRecording#newSnapshotClone incorrectly sets the
> >>>>>>>>>>>> destination in
> >>>>>>>>>>>> the clone, which then triggers a dump prematurely (when
> >>>>>>>>>>>> stop is called).
> >>>>>>>>>>>>
> >>>>>>>>>>>> Purpose of that method is just to take a snapshot of
> >>>>>>>>>>>> existing chunks in
> >>>>>>>>>>>> the disk repository (or from memory). and then let
> >>>>>>>>>>>> dumpStopped(path) do
> >>>>>>>>>>>> the actual dump (copy out the data from the disk repository).
> >>>>>>>>>>>>
> >>>>>>>>>>>> To make it work, some other changes would be needed as
> >>>>>>>>>>>> well, but I don't
> >>>>>>>>>>>> have time to investigate.
> >>>>>>>>>>>>
> >>>>>>>>>>>> Erik
> >>>>>>>>>>>>
> >>>>>>>>>>>>> Hi all,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Please review this change.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> JBS: https://bugs.openjdk.java.net/browse/JDK-8220657
> >>>>>>>>>>>>> webrev:
> >>>>>>>>>>>>> http://cr.openjdk.java.net/~ysuenaga/JDK-8220657/webrev.00/
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> The user might not get flight record via JFR.dump if JFR
> >>>>>>>>>>>>> is started
> >>>>>>>>>>>>> with filename option on target process.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Please see JBS if you want to know how to reproduce.
> >>>>>>>>>>>>>
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Thanks,
> >>>>>>>>>>>>>
> >>>>>>>>>>>>> Yasumasa
> >>>>>>>>>>
> >>>>>>
> >>>
>
>
More information about the hotspot-jfr-dev
mailing list