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

Sat Jan 26 07:37:50 UTC 2019

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