PING: RFR: 8220657: JFR.dump does not work when filename is set
Erik Gahlin
erik.gahlin at oracle.com
Thu Jun 13 07:26:42 UTC 2019
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