RFR(M): 8028128: Add a type safe alternative for working with counter based data
Markus Gronlund
markus.gronlund at oracle.com
Thu Nov 21 10:49:31 UTC 2013
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-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& 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
>>
More information about the hotspot-gc-dev
mailing list