Hi David, Thanks for taking a look again.
Once transitioned from the external API boundary, TracingTime is/can be used internally in the tracing framework. I would like to keep this possibility, so I will leave it in. I will considered this Reviewed if you don't disagree then? Many thanks for your help again Cheers Markus -----Original Message----- From: David Holmes Sent: den 20 november 2013 23:11 To: Markus Gronlund Cc: serviceability-dev; hotspot-gc-...@openjdk.java.net; hotspot-runtime-...@openjdk.java.net Subject: Re: RFR(M): 8028128: Add a type safe alternative for working with counter based data 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 >>
