PING: RFR: [JDK-8216565] Specifying the same path creates a new directory in JFR.configure
Erik Gahlin
erik.gahlin at oracle.com
Fri Sep 20 00:52:06 UTC 2019
Hi Chihiro,
I have tried your fix and it seems to work.
I’m on vacation, so if Yasumasa like to sponsor your fix, that’s fine for me.
Otherwise, I can do it at the end of next week.
While it doesn’t necessary need to be part of this fix, equals and compareTo should be consistent. As you noted in the offline discussion, the compareTo method of SafePath is dead code. So it would be best to remove the compareTo method.
Thanks
Erik
> On 19 Sep 2019, at 16:59, Chihiro Ito <chiroito107 at gmail.com> wrote:
>
> Hi Yasumasa,
>
> This change has already passed the Tier 1 and jdk/jdk/jfr tests.
>
> Regards,
> Chihiro
>
> 2019年9月19日(木) 23:46 Yasumasa Suenaga <yasuenag at gmail.com <mailto:yasuenag at gmail.com>>:
> Hi Chihiro,
>
> >> But your change is based on few months ago. Can you run jtreg test again?
> >> I will push it if jtreg test are passed.
>
> Have you tested your change? - especially tier 1 tests and jdk/jdk/jfr tests.
> If they are fine, I will push your change to jdk/jdk.
>
>
> Yasumasa
>
>
> On 2019/09/15 21:28, Yasumasa Suenaga wrote:
> >>> >>>>> JBS:
> >>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8216565 <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> <mailto: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 <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 <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/ <http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.03/>
> >>> >>>>>
> >>> >>>>> JBS:
> >>> >>>>> https://bugs.openjdk.java.net/browse/JDK-8216565 <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 <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/ <http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.02/>
> >>> >>>>>>>
> >>> >>>>>>> JBS:
> >>> >>>>>>> https://bugs.openjdk.java.net/browse/JDK-8216565 <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> <mailto: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/ <http://cr.openjdk.java.net/~cito/JDK-8216565/webrev.01/>
> >>> >>>>>>>>>>>>
> >>> >>>>>>>>>>>> JBS:
> >>> >>>>>>>>>>>> https://bugs.openjdk.java.net/browse/JDK-8216565 <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/ <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