RFR: 8151438: UL instantiates duplicate tag sets (original) (raw)

Marcus Larsson marcus.larsson at oracle.com
Wed Mar 23 15:22:15 UTC 2016

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 NOTAG terminates the sequence of tags. If other tags (other than NOTAG) 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::logstart() 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 NOTAG 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 othername[512]; 192 other->label(othername, sizeof(othername), ","); 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 tsname, othername); It might be good to check if (!equals) before setting up the othername. Maybe: if (!equals) { char othername[512]; other->label(othername, sizeof(othername), ","); 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 rununittest(Testloglength); 71 rununittest(Testconfigurestdout); 72 rununittest(Testlogconfigurationsubscribe); 73 rununittest(Testtagsetduplicates); I think we should clean this up (in another RFE) by naming these functions similar to the other test functions: 70 rununittest(TestLogLengthtest); 71 rununittest(TestLogConfigureStdouttest); 72 rununittest(TestLogConfigurationSubscribetest); 73 rununittest(TestLogTagSetDuplicatestest); 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 Testlogtagsetduplicates instead of Testtagsetduplicates 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

More information about the hotspot-gc-dev mailing list