RFR(M): 8028128: Add a type safe alternative for working with counter based data

[Markus Gronlund]

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
>