RFR: 8151438: UL instantiates duplicate tag sets

[Bengt Rutisson]

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