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

Wed Nov 20 01:55:32 PST 2013

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; 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,

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:vitalyd at gmail.com]
> *Sent:* den 13 november 2013 02:19
> *To:* Markus Gronlund
> *Cc:* hotspot-runtime-dev at openjdk.java.net; serviceability-dev; 
> hotspot-gc-dev at openjdk.java.net
> *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" <vitalyd at gmail.com 
> <mailto:vitalyd at gmail.com>> 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" 
> <markus.gronlund at oracle.com <mailto:markus.gronlund at oracle.com>> 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:vitalyd at gmail.com 
> <mailto:vitalyd at gmail.com>]
> *Sent:* den 12 november 2013 15:37
> *To:* Markus Gronlund
> *Cc:* serviceability-dev; hotspot-runtime-dev at openjdk.java.net
> <mailto:hotspot-runtime-dev at openjdk.java.net>;
> hotspot-gc-dev at openjdk.java.net 
> <mailto:hotspot-gc-dev at openjdk.java.net>
> *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" <markus.gronlund at oracle.com 
> <mailto:markus.gronlund at oracle.com>> 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
>