[prev in list] [next in list] [prev in thread] [next in thread] 

List:       openjdk-serviceability-dev
Subject:    Re: RFR: 8151438: UL instantiates duplicate tag sets
From:       Marcus Larsson <marcus.larsson () oracle ! com>
Date:       2016-03-29 7:32:46
Message-ID: 56FA2F9E.6030608 () oracle ! com
[Download RAW message or body]

Thanks for reviewing, Stefan!


On 03/23/2016 08:34 PM, Stefan Karlsson wrote:
> Hi Marcus,
> 
> On 23/03/16 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?
> 
> Sounds like an OK plan to me.
> 
> > 
> > For now, I renamed the test to Test_logtagset_duplicates instead of 
> > Test_tagset_duplicates to better indicate that it's a logging test.
> 
> OK.
> 
> > 
> > New webrev:
> > http://cr.openjdk.java.net/~mlarsson/8151438/webrev.01/
> > 
> > Incremental:
> > http://cr.openjdk.java.net/~mlarsson/8151438/webrev.00-01/
> 
> I now see that I proposed if (!equals) but you did the right thing to 
> use if (equals). :)
> 
> Looks good.
> 
> > 
> > Thanks,
> > Marcus
> > 
> > > 
> > > Thanks,
> > > StefanK
> > > 
> > > > 
> > > > Issue:
> > > > https://bugs.openjdk.java.net/browse/JDK-8151438
> > > > 
> > > > Testing:
> > > > New internal VM test through RBT.
> > > > 
> > > > Thanks,
> > > > Marcus
> > > 
> > 
> 


[prev in list] [next in list] [prev in thread] [next in thread]