PING: RFR: [JDK-8216565] Specifying the same path creates a new directory in JFR.configure
Yasumasa Suenaga
yasuenag at gmail.com
Sun Sep 15 12:28:21 UTC 2019
>> >>>>> JBS:
>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8216565
This ticket has rt-notfor-14 label.
I will remove it when I push.
On 2019/09/15 21:25, Yasumasa Suenaga wrote:
> Hi Chihiro,
>
> I will sponsor for you.
> But your change is based on few months ago. Can you run jtreg test again?
> I will push it if jtreg test are passed.
>
>
> Yasumasa
>
>
> On 2019/09/15 2:10, Chihiro Ito wrote:
>> Hi Erik,
>>
>> How is this progressing?
>>
>> Regards,
>> Chihiro
>>
>> 2019年2月2日(土) 13:48 Chihiro Ito <chihiro.ito at oracle.com <mailto:chihiro.ito at oracle.com>>:
>>
>> Erik,
>>
>> Thanks.
>>
>> Regards,
>> Chihiro
>>
>> On 2019/02/01 5:28, Erik Gahlin wrote:
>> > Yes, I can sponsor. I will try to push it this week, otherwise the next.
>> >
>> > Thanks
>> > Erik
>> >
>> >> Hi Erik,
>> >>
>> >> Could you sponsor this?
>> >>
>> >> http://cr.openjdk.java.net/~cito/JDK-8216565/JDK-8216565-changeset
>> >>
>> >> Regards,
>> >> Chihiro
>> >>
>> >> On 2019/01/26 16:37, Chihiro Ito wrote:
>> >>> I uploaded changeset file as the following URL. Could you push it,
>> >>> please?
>> >>>
>> >>> http://cr.openjdk.java.net/~cito/JDK-8216565/JDK-8216565-changeset
>> >>>
>> >>> Regards,
>> >>> Chihiro
>> >>>
>> >>> On 2019/01/26 10:25, Yasumasa Suenaga wrote:
>> >>>> Looks good!
>> >>>>
>> >>>> Erik, can you sponsor this?
>> >>>> I'm not an Oracle employee. (I can sponsor this if I can)
>> >>>>
>> >>>>
>> >>>> Yasumasa
>> >>>>
>> >>>>
>> >>>> On 2019/01/26 1:50, Chihiro Ito wrote:
>> >>>>> Hi Yasumasa,
>> >>>>>
>> >>>>> I changed it to check that the log is contained instead of
>> >>>>> comparing object ids.
>> >>>>>
>> >>>>> Could you review this again, please? If change looks good, please
>> >>>>> push to the repository.
>> >>>>>
>> >>>>> I have run the changes through test-tier1 test sets and saw no
>> >>>>> sign of regression.
>> >>>>>
>> >>>>> webrev:
>> >>>>> http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.03/
>> >>>>>
>> >>>>> JBS:
>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8216565
>> >>>>>
>> >>>>> Regards,
>> >>>>> Chihiro
>> >>>>>
>> >>>>> On 2019/01/24 10:13, Yasumasa Suenaga wrote:
>> >>>>>> Hi Chihiro,
>> >>>>>>
>> >>>>>> I don't like testRepository().
>> >>>>>>
>> >>>>>> As I pointed in [1], time resolution for filename is seconds.
>> >>>>>> Is this test valid?
>> >>>>>>
>> >>>>>> ```
>> >>>>>> + JcmdHelper.jcmd("JFR.configure",
>> >>>>>> REPOSITORYPATH_SETTING_1);
>> >>>>>> + SafePath initialPath =
>> >>>>>> Repository.getRepository().getRepositoryPath();
>> >>>>>> + JcmdHelper.jcmd("JFR.configure",
>> >>>>>> REPOSITORYPATH_SETTING_1);
>> >>>>>> ```
>> >>>>>>
>> >>>>>> If above two jcmd call finish within a second, I think it cannot
>> >>>>>> check this fix.
>> >>>>>>
>> >>>>>> Your change has log output if same path is passed to JFR.configure.
>> >>>>>> For example, TestJcmdChangeLogLevel checks log output for jfr.
>> >>>>>> You might use same way.
>> >>>>>>
>> >>>>>>
>> >>>>>> Thanks,
>> >>>>>>
>> >>>>>> Yasumasa
>> >>>>>>
>> >>>>>>
>> >>>>>> [1]
>> >>>>>> https://mail.openjdk.java.net/pipermail/hotspot-jfr-dev/2019-January/000360.html
>> >>>>>>
>> >>>>>>
>> >>>>>> On 2019/01/24 1:59, Chihiro Ito wrote:
>> >>>>>>> Hi Erik, Yasumasa
>> >>>>>>>
>> >>>>>>> Thank you for your advice. I implemented only checking paths.
>> >>>>>>> Because I wouldn't like to constraints that two objects are
>> >>>>>>> identical.
>> >>>>>>>
>> >>>>>>> Could you review this, please? If change looks good, please push
>> >>>>>>> to the repository.
>> >>>>>>>
>> >>>>>>> I have run the changes through test-tier1 test sets and saw no
>> >>>>>>> sign of regression.
>> >>>>>>>
>> >>>>>>> webrev:
>> >>>>>>> http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.02/
>> >>>>>>>
>> >>>>>>> JBS:
>> >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8216565
>> >>>>>>>
>> >>>>>>> Regards,
>> >>>>>>> Chihiro
>> >>>>>>>
>> >>>>>>>
>> >>>>>>> On 2019/01/22 22:55, Erik Gahlin wrote:
>> >>>>>>>> Hi Chihiro,
>> >>>>>>>>
>> >>>>>>>>> Hi Erik and Yasumasa,
>> >>>>>>>>>
>> >>>>>>>>> I am considering what is the best test for this fix. please
>> >>>>>>>>> give me a advice for following question.
>> >>>>>>>>>
>> >>>>>>>>> Do we need to verify that two objects are same one ?
>> >>>>>>>>
>> >>>>>>>> I assumed you added the check to prevent regressions.
>> >>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> This test verifies that it will not change when the same path
>> >>>>>>>>> is specified in JFR.configure. As long as the same path is
>> >>>>>>>>> specified, even if the results of
>> >>>>>>>>> Repository.getRepository().getRepositoryPath() are different
>> >>>>>>>>> objects, these objects should show the same path. We will not
>> >>>>>>>>> need to verify that it is the same object.
>> >>>>>>>>>
>> >>>>>>>> If you run the test on JDK 12, it will change the path (object)
>> >>>>>>>> even if it is the same (from an equals standpoint). So a object
>> >>>>>>>> check will ensure that the problem has been fixed in JDK 13.
>> >>>>>>>> That said, I could live without a test, but if you add one. I
>> >>>>>>>> don't think we should use Thread.sleep(2000) as this will slow
>> >>>>>>>> down testing.
>> >>>>>>>>
>> >>>>>>>> Erik
>> >>>>>>>>
>> >>>>>>>>> Regards,
>> >>>>>>>>> Chihiro
>> >>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>> On 2019/01/15 22:08, Yasumasa Suenaga wrote:
>> >>>>>>>>>> Hi,
>> >>>>>>>>>>
>> >>>>>>>>>> On 2019/01/15 2:29, Erik Gahlin wrote:
>> >>>>>>>>>>> Hi Chihiro,,
>> >>>>>>>>>>> It would better (and safer) to just compare the references
>> >>>>>>>>>>> than using System.identityHashCode.
>> >>>>>>>>>>
>> >>>>>>>>>> Does "safer" means two paths points different path?
>> >>>>>>>>>> If so, I think you can add sleep between two JFR.configure
>> >>>>>>>>>> calls.
>> >>>>>>>>>>
>> >>>>>>>>>> The resolution of time in Repository::REPO_DATE_FORMAT is
>> >>>>>>>>>> seconds.
>> >>>>>>>>>> IMHO you can test this change "safely" with sleep 1 sec or
>> >>>>>>>>>> longer.
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>> Thanks,
>> >>>>>>>>>>
>> >>>>>>>>>> Yasumasa
>> >>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>>> Asserts.assertTrue(initialPath == samePath));
>> >>>>>>>>>>> …
>> >>>>>>>>>>> Asserts.assertFalse(initialPath == changedPath));
>> >>>>>>>>>>>
>> >>>>>>>>>>> Thanks
>> >>>>>>>>>>> Erik
>> >>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>>> On 13 Jan 2019, at 10:59, Chihiro Ito
>> >>>>>>>>>>>> <chihiro.ito at oracle.com <mailto:chihiro.ito at oracle.com>> wrote:
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> Hi,
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> Thank you for reviewing.
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> I fix test case and if-statement. Could you review this
>> >>>>>>>>>>>> again ?
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> webrev:
>> >>>>>>>>>>>> http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.01/
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> JBS:
>> >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8216565
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> Regards,
>> >>>>>>>>>>>> Chihiro
>> >>>>>>>>>>>>
>> >>>>>>>>>>>> On 2019/01/12 11:05, Yasumasa Suenaga wrote:
>> >>>>>>>>>>>>> Hi Chihiro,
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> The condition in if-statement is a little verbosely.
>> >>>>>>>>>>>>> I think it becomes more simple if you implement
>> >>>>>>>>>>>>> SafePath::equals and use it.
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> Thanks,
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> Yasumasa
>> >>>>>>>>>>>> On 2019/01/12 2:11, Chihiro Ito wrote:
>> >>>>>>>>>>>>> Hi Erik,
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> Thank you for your review. I didn't have this idea.
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> I'll fix it soon and request a review again.
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> Regards,
>> >>>>>>>>>>>>> Chihiro
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>> On 2019/01/12 1:24, Erik Gahlin wrote:
>> >>>>>>>>>>>>>> Hi,
>> >>>>>>>>>>>>>>
>> >>>>>>>>>>>>>> Code looks good, but the 2 s sleep in the test unfortunate.
>> >>>>>>>>>>>>>>
>> >>>>>>>>>>>>>> Perhaps you could compare object identity instead? If it
>> >>>>>>>>>>>>>> hasn't change, it's fine
>> >>>>>>>>>>>>>>
>> >>>>>>>>>>>>>> Erik
>> >>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> Hi all,
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> I reported a following behabior as a bug. [JDK-8216565]
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> Could I have a reviews for small patch, please ?
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> --
>> >>>>>>>>>>>>>>> Even if you specify the same path as repositorypath in
>> >>>>>>>>>>>>>>> JFR.configure, a new subdirectory is created every time.
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> $ ${JAVA_HOME}/bin/jcmd 17535 JFR.configure
>> >>>>>>>>>>>>>>> repositorypath=/tmp
>> >>>>>>>>>>>>>>> 17535:
>> >>>>>>>>>>>>>>> Repository path: /tmp/2019_01_12_00_28_53_17535
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> $ ${JAVA_HOME}/bin/jcmd 17535 JFR.configure
>> >>>>>>>>>>>>>>> repositorypath=/tmp
>> >>>>>>>>>>>>>>> 17535:
>> >>>>>>>>>>>>>>> Repository path: /tmp/2019_01_12_00_29_03_17535
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> If the same directory is specified, you should not
>> >>>>>>>>>>>>>>> create a new .
>> >>>>>>>>>>>>>>> --
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> Regards,
>> >>>>>>>>>>>>>>> Chihiro
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>> On 2019/01/10 1:12, Chihiro Ito wrote:
>> >>>>>>>>>>>>>>>> Hi all,
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>> Could I have a reviews for this fix of "jcmd
>> >>>>>>>>>>>>>>>> JFR.configure", please ?
>> >>>>>>>>>>>>>>>> This patch passed all tests in test-tier1.
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>> http://cr.openjdk.java.net/~cito/webrev.00/
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>> In current implementation even if user specify the same
>> >>>>>>>>>>>>>>>> repository base path in jcmd JFR.configure, hotspot
>> >>>>>>>>>>>>>>>> create a new directory (base path + timestamp) as
>> >>>>>>>>>>>>>>>> repository.
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>> This patch is hotspot don't create new directory and
>> >>>>>>>>>>>>>>>> hotspot keep to use current one.
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>> Regards,
>> >>>>>>>>>>>>>>>> Chihiro
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>
>> >>>>>>>>>>>>>>
>> >>>>>>>>>>>>>
>> >>>>>>>>>>>>
>> >>>>>>>>>>>
>> >>>>>>>>>>
>> >>>>>>>>>
>> >>>>>>>>
>> >>>>>>>>
>> >>>>>>>
>> >>>>>>
>> >>>>>
>> >>>>
>> >>>
>> >>
>> >
>> >
>>
More information about the hotspot-jfr-dev
mailing list