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

David Holmes david.holmes at oracle.com
Thu Nov 21 05:57:00 PST 2013 

On 21/11/2013 8:49 PM, Markus Gronlund wrote:
> 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.

Ok.


> I will considered this Reviewed if you don't disagree then?

Yes.

> Many thanks for your help again
>

You are welcome. :)

David
> Cheers
> Markus
>
>
>
> -----Original Message-----
> From: David Holmes
> Sent: den 20 november 2013 23:11
> To: Markus Gronlund
> Cc: serviceability-dev; hotspot-gc-dev at openjdk.java.net; hotspot-runtime-dev at 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&amp; time) {}
>> + #59 void set_endtime(const Ticks&amp; 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
>>>

More information about the hotspot-runtime-dev mailing list