RFR: 8151438: UL instantiates duplicate tag sets

Stefan Karlsson stefan.karlsson at oracle.com
Wed Mar 23 10:00:32 UTC 2016

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:


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. 

if (!equals) {
   char other_name[512];
   other->label(other_name, sizeof(other_name), ",");
   assert(!equals /* or just false */, ...);


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.


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

More information about the hotspot-gc-dev mailing list