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:25:25 UTC 2019
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