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
_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
