PING: RFR: [JDK-8216565] Specifying the same path creates a new directory in JFR.configure
Yasumasa Suenaga
yasuenag at gmail.com
Thu Sep 19 14:46:16 UTC 2019
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
>
> 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