RFR: 8151438: UL instantiates duplicate tag sets

[Marcus Larsson]

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
>