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 >
