Hi Markus,
I couldn't quite work out if TracingTime is obsolete after these changes?
Otherwise I have no further comments.
Thanks,
David
On 20/11/2013 10:17 PM, Markus Gronlund wrote:
Hi again,
I have created an updated webrev02 for the feedback I have gotten so far. In
addition, I have added a few improvements to the original suggestion.
Updated Webrev02: http://cr.openjdk.java.net/~mgronlun/8028128/webrev02/
Original Webrev01: http://cr.openjdk.java.net/~mgronlun/8028128/webrev01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8028128
Thanks a lot for your help/patience in reviewing this large change - very much
appreciated.
Cheers
Markus
Description of deltas/updates to Webrev02 compared to Webrev01:
// updates based on feedback
utilities/ticks.inline.hpp
// removed functions comparing against jlong(s)
- #63 inline bool operator==(const Ticks& lhs, jlong compare_value) {
- #64 return lhs.value() == compare_value;
- #65 }
- #89 inline bool operator!=(const Ticks& lhs, jlong compare_value) {
- #90 return lhs.value() != compare_value;
- #91 }
// updated to gt signs for operator<= functions
- #52 return !operator<(lhs, rhs);
+ #52 return !operator >(lhs, rhs);
- #98 return !operator<(lhs,rhs);
+ #98 return !operator>(lhs,rhs);
// thanks to Vitaly Davidovich.
share/vm/gc_implementation/gcTimer.hpp
// removed default parameter for report_gc_phase_* functions
- #114 void report_gc_phase_start(const char* name, const Ticks& time =
Ticks::now());
+ #114 void report_gc_phase_start(const char* name, const Ticks& time);
- #115 void report_gc_phase_end(const Ticks& time = Ticks::now());
+ #115 void report_gc_phase_end(const Ticks& time);
- #145 void register_gc_phase_start(const char* name, const Ticks& time =
Ticks::now());
+ #145 void register_gc_phase_start(const char* name, const Ticks& time);
- #146 void register_gc_phase_end(const Ticks& time = Ticks::now());
+ #146 void register_gc_phase_end(const Ticks& time);
// thanks to Per Liden.
//
//
//
// Additional changes compared to webrev01 (delta)
//
webrev01 still allows a user to pass in a jlong (knowingly and/or unknowingly)
for event.starttime() and event.endtime().
For example:
event.set_endtime(os::elapsed_counter()); // direct call using
os::elapsed_counter()
This can be avoided by increasing encapsulation:
src/share/vm/trace/traceEvent.hpp:
set_starttime(const TracingTime& time) // remove default parameter and make
protected
set_endtime(const TracingTime& time) // remove default parameter and make
protected
Now, the only visible external API for setting the start and/or endtime of an event is by
passing a "Ticks" type (a compiler enforced constraint):
Using the same example as above, now if attempting to pass a jlong, like a
direct call using os::elapsed_counter(), this now yields (at compile time):
..\..\src\share\vm\gc_implementation\shared\gcTraceSend.cpp(63): error C2248:
'TraceEvent<T>::set_endtime' : cannot access protected member declared in class
'TraceEvent<T>'
In order to make these functions protected, i have also had to update the class
loader events in SystemDictionary and ClassLoaderDataGraph:
share/vm/classfile/SystemDictionary.hpp:
// remove include file:
- #34 #include "trace/traceTime.hpp"
+ #80 forward declare class Ticks;
// update the signature
- #640 static void post_class_load_event(const TracingTime& start_time,
instanceKlassHandle k,
Handle initiating_loader);
+ #640 static void post_class_load_event(const Ticks& start_time,
instanceKlassHandle k,
Handle initiating_loader);
share/vm/classfile/SystemDictionary.cpp:
// added include files
+#58 #include "utilities/macros.hpp"
+#59 #include "utilities/ticks.hpp"
// removed old include file:
- #61 #include "trace/traceMacros.hpp"
// updated type
- #601 TracingTime class_load_start_time = Tracing::time();
+ #601 Ticks class_load_start_time = Ticks::now();
// updated type
- #1009 TracingTime class_load_start_time = Tracing::time();
+ #1009 Ticks class_load_start_time = Ticks::now();
// updated signature
- #2668 void SystemDictionary::post_class_load_event(TracingTime start_time,
instanceKlassHandle k, Handle initiating_loader)
+ #2668 void SystemDictionary::post_class_load_event(const Ticks& start_time,
instanceKlassHandle k, Handle initiating_loader)
// removed setting the endtime
- #2674 event.set_endtime(Tracing::time());
src/share/vm/classfile/classLoaderData.hpp:
// update include
- #36 #include "trace/traceTime.hpp"
+ #36 #include "utilities/ticks.hpp"
// updated type for _class_unload_time;
- #101 static TracingTime _class_unload_time;
+ #101 static Ticks _class_unload_time;
src/share/vm/classfile/classLoaderData.cpp:
+ #65 #include "utilities/macros.hpp"
// updated timestamp
- #757 _class_unload_time = Tracing::time();
+ #757 _class_unload_time = Ticks::now();
// updated type definition
- #835 TracingTime ClassLoaderDataGraph::_class_unload_time;
+ #835 Ticks ClassLoaderDataGraph::_class_unload_time;
src/share/vm/trace/traceEventClasses.xls:
// update the !INCLUDE_TRACE stubs
- #59 void set_starttime(jlong time) const {}
- #60 void set_endtime(jlong time) const {}
+ #59 void set_starttime(const Ticks& time) {}
+ #59 void set_endtime(const Ticks& time) {}
Thanks
Markus
Greetings,
Kindly asking for reviews for the following changeset:
Webrev: http://cr.openjdk.java.net/~mgronlun/8028128/webrev01/
Bug: https://bugs.openjdk.java.net/browse/JDK-8028128
*Description:*
Currently, when working with counter based data, the primary data type
representation in the VM are raw jlong's. jlong's are employed to
represent both:
1. a single instance in time, for example a "now" timestamp 2. a time
duration, a timespan, for example the duration between "now" - "then"
Having a raw jlong type represent both of these shifts the
responsibility to the programmer to always ensure the correct validity
of the counter data, without any assistance either at compile time or
at runtime.
The event based tracing framework relies heavily on counter based data
in order to keep track of time and timing related information and we
have seen counter data errors where the representation sent to the
tracing framework has submitted the wrong time, i.e. a timespan when a
time instant was expected and vice versa.
We should therefore add an alternative to jlongs for representing
counter based data. We should enlist the help of the type system to
enforce the correct data types at compile time as well as introduce
strong runtime asserts in order to help with the correct usage of time
and to reduce the number of potential bugs.
I will therefore add two new types in src/share/vm/utilities:
1. "Ticks" - represents a single counter based timestamp, a single
instance in time.
2. "Tickspan" - represents a counter based duration, a timespan,
between two "Ticks"
Please see the added files:
-src/share/vm/utilities/ticks.hpp
-src/share/vm/utilities/ticks.inline.hpp
-src/share/vm/utilities/ticks.cpp
I have also updated some of the existing event based tracing "event
sites" to employ usage of these new types as well as updated a few
places in the event based tracing framework implementation to
accommodate these types.
*Testing completed:*
nsk.stress.testlist (UTE/Aurora)
GC nightlies (Aurora)
gc/gctests/* (UTE/Aurora)
nsk.gc.testlist (UTE/Aurora)
SpecJBB2013 (no regression)
SpecJBB2005 (no regression)
Kitchensink (locally Windows) runs for +> 9 days (still running.)
Thanks in advance
Markus
