PING: RFR: [JDK-8216565] Specifying the same path creates a new directory in JFR.configure

Chihiro Ito chihiro.ito at oracle.com
Sat Feb 2 03:28:58 UTC 2019 

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> 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