RFR: 8151438: UL instantiates duplicate tag sets

[Marcus Larsson]

Thanks for reviewing, Bengt!

Marcus

On 03/23/2016 01:36 PM, Bengt Rutisson wrote:
>
> Hi Marcus,
>
> Looks good to me.
>
> Thanks,
> Bengt
>
> On 2016-03-23 11:59, Marcus Larsson wrote:
>> Hi Stefan,
>>
>> On 03/23/2016 11:00 AM, Stefan Karlsson wrote:
>>> Hi Marcus,
>>>
>>> On 2016-03-23 10:23, Marcus Larsson wrote:
>>>> Hi,
>>>>
>>>> Please review the following patch to fix the issue where duplicate 
>>>> tagsets are created for the same logical tagset.
>>>>
>>>> The code that emulates the variadic template arguments assumes that 
>>>> _NO_TAG terminates the sequence of tags. If other tags (other than 
>>>> _NO_TAG) follow a terminating tag, template instances that are 
>>>> otherwise considered equal (since they share tags up until the 
>>>> terminating tag), might not be considered equal in the template 
>>>> sense (one of the template arguments can differ). This would cause 
>>>> another template instantiation for the same logical tagset and we 
>>>> end up with logical duplicates.
>>>>
>>>> The if-statement to append the 'start' tag in 
>>>> GCTraceTimeImpl::log_start() caused such problematic template 
>>>> instantiations, so any tagset used with GCTraceTime would be 
>>>> duplicated. To fix this, the template instantiation has been forced 
>>>> to only be made once, ensuring no real tag follows the first 
>>>> _NO_TAG by using the ternary operator.
>>>>
>>>> This patch also includes a test checking for invalid tagsets like 
>>>> these, and also checks for tagsets that are just permutations of 
>>>> other tagsets. Such tagsets should be avoided to prevent confusion, 
>>>> and to reduce overhead. (The test exposed one case where a 
>>>> different permutation was used, so I've fixed that as well.)
>>>>
>>>> Webrev:
>>>> http://cr.openjdk.java.net/~mlarsson/8151438
>>>
>>> The change looks good. I have a couple of comments about the test:
>>>
>>> http://cr.openjdk.java.net/~mlarsson/8151438/webrev.00/src/share/vm/logging/log.cpp.frames.html 
>>>
>>>
>>> 191 char other_name[512];
>>> 192 other->label(other_name, sizeof(other_name), ",");
>>> 193 // Since tagsets are implemented using template arguments, using 
>>> both of
>>> 194 // the (logically equivalent) tagsets (t1, t2) and (t2, t1) 
>>> somewhere will
>>> 195 // instantiate two different LogTagSetMappings. This causes 
>>> multiple
>>> 196 // tagset instances to be created for the same logical set. We 
>>> want to
>>> 197 // avoid this to save time, memory and prevent any confusion 
>>> around it.
>>> 198 assert(!equal, "duplicate LogTagSets found: '%s' vs '%s' "
>>> 199 "(tags must always be specified in the same order for each 
>>> tagset)",
>>> 200 ts_name, other_name);
>>>
>>>
>>> It might be good to check if (!equals) before setting up the 
>>> other_name. Maybe:
>>>
>>> if (!equals) {
>>>   char other_name[512];
>>>   other->label(other_name, sizeof(other_name), ",");
>>>   assert(!equals /* or just false */, ...);
>>> }
>>>
>>
>> Fixed.
>>
>>>
>>> http://cr.openjdk.java.net/~mlarsson/8151438/webrev.00/src/share/vm/utilities/internalVMTests.cpp.frames.html 
>>>
>>>
>>> The test for the logging framework doesn't have a good prefix:
>>>
>>>   70   run_unit_test(Test_log_length);
>>>   71   run_unit_test(Test_configure_stdout);
>>>   72   run_unit_test(Test_logconfiguration_subscribe);
>>>    73 run_unit_test(Test_tagset_duplicates);
>>>
>>> I think we should clean this up (in another RFE) by naming these 
>>> functions similar to the other test functions:
>>>
>>>   70   run_unit_test(TestLogLength_test);
>>>   71   run_unit_test(TestLogConfigureStdout_test);
>>>   72   run_unit_test(TestLogConfigurationSubscribe_test);
>>>    73 run_unit_test(TestLogTagSetDuplicates_test);
>>>
>>> I understand that there are some inconsistent names in the test 
>>> list, but I think we should start by fixing the names for the 
>>> logging tests.
>>
>> I agree. Although I would like these tests to be ported to the unit 
>> test framework once that's been integrated. It will allow better 
>> organization and grouping of tests. Perhaps we should leave it as is 
>> until then?
>>
>> For now, I renamed the test to Test_logtagset_duplicates instead of 
>> Test_tagset_duplicates to better indicate that it's a logging test.
>>
>> New webrev:
>> http://cr.openjdk.java.net/~mlarsson/8151438/webrev.01/
>>
>> Incremental:
>> http://cr.openjdk.java.net/~mlarsson/8151438/webrev.00-01/
>>
>> Thanks,
>> Marcus
>>
>>>
>>> Thanks,
>>> StefanK
>>>
>>>>
>>>> Issue:
>>>> https://bugs.openjdk.java.net/browse/JDK-8151438
>>>>
>>>> Testing:
>>>> New internal VM test through RBT.
>>>>
>>>> Thanks,
>>>> Marcus
>>>
>>
>