Hi David, I'm sorry I haven't got around to updating and re-posting a new version for the webrev :(
Thanks very much for reviewing. I intend to send out an updated webrev which will contain the updates from the feedback I have gotten so far. In addition, I have a few improvements to the original suggestion that will be included in the updated webrev. I will include a detailed list of where the deltas are compared to the previous webrev as well to make life a bit easier :) Thanks again Markus -----Original Message----- From: David Holmes Sent: den 19 november 2013 02:58 To: Markus Gronlund Cc: serviceability-dev; [email protected]; [email protected] Subject: Re: RFR(M): 8028128: Add a type safe alternative for working with counter based data Hi Markus, On 13/11/2013 7:46 PM, Markus Gronlund wrote: > Hi again! > > Thank you for spotting these! I'm sorry, silly mistakes. > > I have updated lines 51-53 to: > > 51 inline bool operator<=(const Tickspan& lhs, const Tickspan& rhs) > { > > 52 return !operator>(lhs,rhs); > > 53 } > > And lines 97-99 as: > > 97 inline bool operator<=(const Ticks& lhs, const Ticks& rhs) { > > 98 return !operator>(lhs,rhs); > > 99 } Would have been nice to generate an updated webrev for these fixes for late reviewers :) I don't have any further comments. Reviewed. Thanks, David > > > > > Thanks Vitaly! > > Cheers > > Markus > > *From:*Vitaly Davidovich [mailto:[email protected]] > *Sent:* den 13 november 2013 02:19 > *To:* Markus Gronlund > *Cc:* [email protected]; serviceability-dev; > [email protected] > *Subject:* RE: RFR(M): 8028128: Add a type safe alternative for > working with counter based data > > Similar issue with the Ticks versions. > > Sent from my phone > > On Nov 12, 2013 8:17 PM, "Vitaly Davidovich" <[email protected] > <mailto:[email protected]>> wrote: > > Yeah, naming things is often a pain. > > 51 inline bool operator<=(const Tickspan& lhs, const Tickspan& rhs) { > 52 return !operator<(lhs,rhs); > 53 } > 54 > 55 inline bool operator>=(const Tickspan& lhs, const Tickspan& rhs) { > 56 return !operator<(lhs,rhs); > 57 } > > These look like bugs; first one should probably be !operator>(lhs,rhs) > and !operator<(lhs,rhs), respectively. > > Sent from my phone > > On Nov 12, 2013 10:32 AM, "Markus Gronlund" > <[email protected] <mailto:[email protected]>> wrote: > > Hi Vitaly, > > Thanks for you input! > > "You have some operator overloads for these that can compare them > against jlongs - doesn't that still make it easy to mix up what you're > comparing? Personally, since these are value types I'd make their API > work only against these new types and make caller create the right > type for comparison." > > Thanks for spotting these - the compare against jlongs in from an > older version of the webrev where this was allowed - you are right, it > should not be allowed, I thought I had updated this, will remove them > - many thanks for spotting. > > As for the names, > > "Ticks and Tickspan don't seem too optimal of names. What about > TimeInstance and TimeSpan?" > > I agree with you that the names could possibly be improved, believe me > I have tried to use multiple names - one thing I wanted was to > emphasize that these types (currently) are only based on counter's > (i.e ticks), and do not represent any higher "time" abstraction. That > is, the implementation is tied to data from counters (based on ticks > with a specific ticks frequency). In my current context, I use these > values to talk about "time" (as you saw from the original post), but > "time" can mean many things to different people, esp. many > implementations and clock sources. > > Also, there already exist a few "Time/Timer" types (although they are > not as good as I want them to be for current purposes), for example > there is a type called Timestamp (in runtime/timers.hpp). > > I have come to the conclusion that in order to address the overall > time abstractions in Hotspot, we would need to make a larger effort - > this we should indeed do, but it's outside the current scope. > > Also I am pretty sure that when we do revisit the time abstractions, > the names for these types will most likely change - that made me leave > the names to emphasize the "ticks" sources. > > Thanks for your input. > > Best regards > > Markus > > *From:*Vitaly Davidovich [mailto:[email protected] > <mailto:[email protected]>] > *Sent:* den 12 november 2013 15:37 > *To:* Markus Gronlund > *Cc:* serviceability-dev; [email protected] > <mailto:[email protected]>; > [email protected] > <mailto:[email protected]> > *Subject:* Re: RFR(M): 8028128: Add a type safe alternative for > working with counter based data > > Hi Markus, > > Ticks and Tickspan don't seem too optimal of names. What about > TimeInstance and TimeSpan? > > You have some operator overloads for these that can compare them > against jlongs - doesn't that still make it easy to mix up what you're > comparing? Personally, since these are value types I'd make their API > work only against these new types and make caller create the right > type for comparison. > > Just my $.02, feel free to ignore :). > > Thanks > > Sent from my phone > > On Nov 12, 2013 6:16 AM, "Markus Gronlund" <[email protected] > <mailto:[email protected]>> wrote: > > 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 >
