RFR: 8151438: UL instantiates duplicate tag sets

[Stefan Karlsson]

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 */, ...);
}


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.

Thanks,
StefanK

>
> Issue:
> https://bugs.openjdk.java.net/browse/JDK-8151438
>
> Testing:
> New internal VM test through RBT.
>
> Thanks,
> Marcus