You are viewing a plain text version of this content. The canonical link for it is here.
Posted to dev@ofbiz.apache.org by Adam Heath <do...@brainfood.com> on 2010/02/15 17:12:45 UTC
TimeDuration is not immutable
The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
however, it's fields are not final, and lots of methods modify it's
fields.
Was there any research into other duration implementations? I see one
at javax.xml.datatype.Duration, that seems mostly compatible, except
that it doesn't have millisecond resolution.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
>> If the internal representation was an array of ints, then we could have
>> int constants that are indexes into the array. Then we would need only
>> one accessor method:
>
> Yeah, and array is what I was thinking too; however, it'll still take
> up more memory than a single long representation. It really comes
> down to which set of methods are called more often,
> compare/hashCode/equals, or the field accessors.
Are you expecting to keep a lot of these in memory? You could also
create an array of shorts - that will accommodate a duration up to
32,000 years.
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>>> Additionally, becase fromLong creates an empty TimeDuration, then sets
>>> the fields one by one, the vm is free to re-order those values, and
>>> even delay then for a *long* time, well after fromLong returns to it's
>>> caller. There is no synchronization on this; if these fields were
>>> only set from the constructor, then this problem wouldn't occur.
>
> Unfortunately, those are JVM details I wasn't aware of at the time.
Yeah, it gets complex. 'new' implies happens-before symantics.
Anything that happens after, can be reorded. If you store duration
into a HashMap, which is unsynchronized, there is still no
happens-before. Even those TimeDuration's hashCode itself will return
the correct value in this case, it'll only be correct for the current
thread. Some other thread might see the wrong hashCode, so won't get
the correct value back from the map.
This all changes if you store into a map that is synchronized(like
Hashtable, or Collections.synchronizedMap, or ConcurrentMap, or
FastMap), but then you have weird, hard-to-track-down bugs.
This is why the Java Concurrency in Practice book says that classes
that could be used as keys should really follow the Immutable pattern,
as then you get the happens-after support that 'new' gives you.
>> Also, it might be nice to make use of TimeUnit for conversion to other
>> values.
>
> Or maybe use the class as inspiration. A convert method might be handy.
>
> If the internal representation was an array of ints, then we could have
> int constants that are indexes into the array. Then we would need only
> one accessor method:
Yeah, and array is what I was thinking too; however, it'll still take
up more memory than a single long representation. It really comes
down to which set of methods are called more often,
compare/hashCode/equals, or the field accessors.
> int days = duration.get(TimeDuration.DAYS);
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
>> Additionally, becase fromLong creates an empty TimeDuration, then sets
>> the fields one by one, the vm is free to re-order those values, and
>> even delay then for a *long* time, well after fromLong returns to it's
>> caller. There is no synchronization on this; if these fields were
>> only set from the constructor, then this problem wouldn't occur.
Unfortunately, those are JVM details I wasn't aware of at the time.
> Also, it might be nice to make use of TimeUnit for conversion to other
> values.
Or maybe use the class as inspiration. A convert method might be handy.
If the internal representation was an array of ints, then we could have
int constants that are indexes into the array. Then we would need only
one accessor method:
int days = duration.get(TimeDuration.DAYS);
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
>>>>> however, it's fields are not final, and lots of methods modify it's
>>>>> fields.
>>>> The methods that modify fields are directly or indirectly called by the
>>>> constructor and are protected. Let me know if any public method modifies
>>>> a field.
>>> fromLong modifies the fields directly, not from the constructor.
>> fromLong is a static method that returns an immutable instance.
>
> Right you are, but the statement still stands. fromLong should use
> the constructor to set that.
>
>>> makeNegative modifies the fields.
>> makeNegative is protected and used internally.
>
> The the same statement for set() applies.
>
>>> set() is protected, TimeDuration is not final, so sub-classes could
>>> change the fields.
>> If I need to extend TimeDuration, then I might also need to change the
>> fields.
>
> If fromLong just used the constructor, after doing math on the long,
> then it wouldn't need to call makeNegative itself. Then, the only
> places that would call set() or makeNegative would be the constructor,
> so the code could be inlined. Once it is inlined, then the fields can
> be made final, and the class becomes a proper immutable.
>
> Additionally, becase fromLong creates an empty TimeDuration, then sets
> the fields one by one, the vm is free to re-order those values, and
> even delay then for a *long* time, well after fromLong returns to it's
> caller. There is no synchronization on this; if these fields were
> only set from the constructor, then this problem wouldn't occur.
>
>>> Ideally, this class should extend Number, or some other number class.
>
> So we can do TimeDuration.longValue(), and get the internal duration.
>
> Also, shouldn't the duration store the value internally as a long?
> That would solve the normalization problem, and also reduce the memory
> usage.
>
>> Why?
>>
>>> It should also normalize it's values. If I set a duration of 86400
>>> seconds, it should normalize to 1 day.
>> Agreed. That was something I always wanted to add, but never got around
>> to it.
>>
>>> Also, the 7 arg constructor incorrectly sets itself negative. If I
>>> set a duration of 5 days, and -30 seconds, it'll think it's negative.
>> If normalization code was written, then that scenario could be
>> accommodated. For now a duration is positive or negative.
>
> I'm willing to do all these changes; first would be to have test cases
> and full coverage.
Also, it might be nice to make use of TimeUnit for conversion to other
values.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> It would be nice if we could do math with those values. If creating a
>>> widget takes 1 minute and 30 seconds, how long would it take to create
>>> one hundred widgets? If a production run of 100 widgets starts at 9:00
>>> AM Thursday, at what time will the production run end? These are the
>>> kind of problems I hoped to solve with this class and others.
>>
>> Maybe pattern it after BigDecimal. The benefit there is that things
>> like groovy will magically work with it.
>>
>> The main issue, is that when a lower field becomes larger, you may not
>> be able to rollover to a larger field. Ie, some days have 61 seconds,
>> some years have an extra day. So once you do math, you may have to
>> carry the mutations around as extra data. Unless someone else has
>> some insights.
>
> You're still mixing a duration of time with a calendar. A duration of
> time must be expressed outside of a specific calendar
>
> It is acceptable to have any value in any of the fields, so don't get
> too hung up on what the fields contain. In the widget example, it would
> be perfectly acceptable to have a duration of 100 minutes and 3000 seconds.
Another way of looking at it: TimeDuration is elapsed time. That is not
the same as wall clock time. You're mixing elapsed time with wall clock
time.
If a sprinter runs the 40 yard dash in 5 seconds, that 5 seconds will
always be 5 seconds - regardless of whether or not he ran it in a leap year.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> It would be nice if we could do math with those values. If creating a
>> widget takes 1 minute and 30 seconds, how long would it take to create
>> one hundred widgets? If a production run of 100 widgets starts at 9:00
>> AM Thursday, at what time will the production run end? These are the
>> kind of problems I hoped to solve with this class and others.
>
> Maybe pattern it after BigDecimal. The benefit there is that things
> like groovy will magically work with it.
>
> The main issue, is that when a lower field becomes larger, you may not
> be able to rollover to a larger field. Ie, some days have 61 seconds,
> some years have an extra day. So once you do math, you may have to
> carry the mutations around as extra data. Unless someone else has
> some insights.
You're still mixing a duration of time with a calendar. A duration of
time must be expressed outside of a specific calendar
It is acceptable to have any value in any of the fields, so don't get
too hung up on what the fields contain. In the widget example, it would
be perfectly acceptable to have a duration of 100 minutes and 3000 seconds.
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> It would be nice if we could do math with those values. If creating a
> widget takes 1 minute and 30 seconds, how long would it take to create
> one hundred widgets? If a production run of 100 widgets starts at 9:00
> AM Thursday, at what time will the production run end? These are the
> kind of problems I hoped to solve with this class and others.
Maybe pattern it after BigDecimal. The benefit there is that things
like groovy will magically work with it.
The main issue, is that when a lower field becomes larger, you may not
be able to rollover to a larger field. Ie, some days have 61 seconds,
some years have an extra day. So once you do math, you may have to
carry the mutations around as extra data. Unless someone else has
some insights.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> Adam Heath wrote:
>>> new TimeDuration(cal, cal) doesn't handle when one cal is 0, and the
>>> other is negative; in detail, if I try to handle a duration with a
>>> border of 0, and a border of -1 month, it comes out quite wrong.
>> Huh? If you're referring to dates prior to the epoch, then yes - there
>> is a design flaw there. I never thought of that before, and it
>> definitely needs to be fixed.
>
> This is the most major actual real bug I've found. As such, I have
> used git to rebase back to before I rewrote it(which I may end up
> throwing away anyways), and am writing just test cases against your
> current code.
>
>>> fromLong(long) does its own internal math, not using any Calendar class.
>>>
>>> What happens with you specify a duration of 3 years, then convert that
>>> to a long? How does leap year/leap second fit into that?
>> It's just an encoding method. I could have done bit shifting instead.
>> Three years will be encoded as three years - leap years/seconds don't
>> have anything to do with it.
>>
>>> Now, if we really do want to stick with this TimeDuration class, the
>>> code will get *very* complex.
>> Why? Aside from the bug mentioned above, the class works. Why are you so
>> determined to change it or eliminate it?
>
> I'm not trying to get rid of it. I'm trying to understand under which
> circumstances it is used.
>
> Remember, I am not you, and haven't seen the same things you have. I
> don't know what caused you to write this, what situation you were in
> when you decided to use a full class for duration management. If I am
> having questions, it's obvious that I don't understand, so don't just
> wave it away.
Fair enough. I'm not feeling well today, so I'm being a little testy. My
apologies.
TimeDuration is one class in a number of classes used for calendaring. A
Work Effort GenericValue, a Temporal Expression, and a TimeDuration
combined together can express any event.
> I've asked quite a few questions about the implementation. Some have
> been bugs, others are changes you hadn't gotten around to doing yet,
> and others are just different ways of doing things. This shows that I
> am understanding some parts, but not others. What we will get out of
> that will be better than what we had before.
I agree. I appreciate the interest you've taken in it.
> The reason I am asking so much, is there may be a better way to do
> things. If there isn't, that's fine. If you've done already done
> reasearch, and finally settled on this system, that's fine too, but
> then you need to tell me why that is. I've shown enough little burrs
> with this code that it makes me want to use some already implemented
> system that has all the bugs worked out, rather then reinventing the
> wheel, and fixing the same set of bugs that someone else has already
> solved.
Agreed.
The main thing to keep in mind is that a duration of time must be
expressed outside of a specific calendar. Take your three years example
- how many days would that represent? There is no way to know unless the
duration is applied to a calendar. Even then the number of days could be
anything - depending on the year and the calendaring system.
If you take the long encoding/decoding out of the class you will see the
essence of what it does. It's a simple container for a set of values.
Those values can be applied to a Calendar instance to derive a date.
It would be nice if we could do math with those values. If creating a
widget takes 1 minute and 30 seconds, how long would it take to create
one hundred widgets? If a production run of 100 widgets starts at 9:00
AM Thursday, at what time will the production run end? These are the
kind of problems I hoped to solve with this class and others.
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> new TimeDuration(cal, cal) doesn't handle when one cal is 0, and the
>> other is negative; in detail, if I try to handle a duration with a
>> border of 0, and a border of -1 month, it comes out quite wrong.
>
> Huh? If you're referring to dates prior to the epoch, then yes - there
> is a design flaw there. I never thought of that before, and it
> definitely needs to be fixed.
This is the most major actual real bug I've found. As such, I have
used git to rebase back to before I rewrote it(which I may end up
throwing away anyways), and am writing just test cases against your
current code.
>> fromLong(long) does its own internal math, not using any Calendar class.
>>
>> What happens with you specify a duration of 3 years, then convert that
>> to a long? How does leap year/leap second fit into that?
>
> It's just an encoding method. I could have done bit shifting instead.
> Three years will be encoded as three years - leap years/seconds don't
> have anything to do with it.
>
>> Now, if we really do want to stick with this TimeDuration class, the
>> code will get *very* complex.
>
> Why? Aside from the bug mentioned above, the class works. Why are you so
> determined to change it or eliminate it?
I'm not trying to get rid of it. I'm trying to understand under which
circumstances it is used.
Remember, I am not you, and haven't seen the same things you have. I
don't know what caused you to write this, what situation you were in
when you decided to use a full class for duration management. If I am
having questions, it's obvious that I don't understand, so don't just
wave it away.
I've asked quite a few questions about the implementation. Some have
been bugs, others are changes you hadn't gotten around to doing yet,
and others are just different ways of doing things. This shows that I
am understanding some parts, but not others. What we will get out of
that will be better than what we had before.
The reason I am asking so much, is there may be a better way to do
things. If there isn't, that's fine. If you've done already done
reasearch, and finally settled on this system, that's fine too, but
then you need to tell me why that is. I've shown enough little burrs
with this code that it makes me want to use some already implemented
system that has all the bugs worked out, rather then reinventing the
wheel, and fixing the same set of bugs that someone else has already
solved.
Another reason I keep asking all these questions, is so that I can
understand the code while writing the test cases. Not only do test
cases help you prove that the code is running correctly, they also
serve as documentation, giving examples of how to use it. For
something like TimeDuration, which can stand alone, without any other
code, writing a set of test cases is extremely simple to do, for the
original author.
ps: This is another one of those emails where I have tried to not
single anyone out. There are things I've said here that apply to
anyone who is writing new code/features.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> new TimeDuration(cal, cal) doesn't handle when one cal is 0, and the
> other is negative; in detail, if I try to handle a duration with a
> border of 0, and a border of -1 month, it comes out quite wrong.
Huh? If you're referring to dates prior to the epoch, then yes - there
is a design flaw there. I never thought of that before, and it
definitely needs to be fixed.
> fromLong(long) does its own internal math, not using any Calendar class.
>
> What happens with you specify a duration of 3 years, then convert that
> to a long? How does leap year/leap second fit into that?
It's just an encoding method. I could have done bit shifting instead.
Three years will be encoded as three years - leap years/seconds don't
have anything to do with it.
> Now, if we really do want to stick with this TimeDuration class, the
> code will get *very* complex.
Why? Aside from the bug mentioned above, the class works. Why are you so
determined to change it or eliminate it?
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
>>> simpler. It could use some TLC.
>> Ok, sorry to say this, but TimeDuration needs to be
>> removed.
>> com.ibm.icu.text.DurationFormat, which handles things like
>> 2 days from
>> now and 3 hours ago, uses a long for the duration.
>>
>> Additionally, trying to duplicate leap year calculation,
>> there is not
>> a whole number of days in a year(it's not quite
>> 365.24). If long is
>> good enough for icu, then why should we try to use
>> something else?
>
> Grasshopper,
>
> TimeDuration is not concerned with such details. They are delegated to the Calendar class.
>
> Tell me, using com.ibm.icu.text.DurationFormat - how do you add two durations?
I'm still waffling.
Still more issues.
new TimeDuration(cal, cal) doesn't handle when one cal is 0, and the
other is negative; in detail, if I try to handle a duration with a
border of 0, and a border of -1 month, it comes out quite wrong.
fromLong(long) does its own internal math, not using any Calendar class.
What happens with you specify a duration of 3 years, then convert that
to a long? How does leap year/leap second fit into that?
You add 2 durations by adding their long values, of course.
DurationFormat considers a duration has a plain, simple long.
Here's another way of looking at it.
Object.wait(and friends) have a timeout parameter, that is a long,
which represents milliseconds. Java1.5 added more variants, that
still take a long, but then also take a TimeUnit enum. In either
case, the long represents a duration of time, and, it's not an object.
Even in the thread pool stuff in java.util.concurrent, they use a
long+TimeUnit pair to represent periods/durations.
Now, if we really do want to stick with this TimeDuration class, the
code will get *very* complex.
If you originally want a duration of 50 billion milliseconds, then
that's exactly what you will get.
However, if you request a duration field that just so happens to have
leap-year/leap-second mutations, then calling toLong on it doesn't
fully make sense. You can only get the correct value when you add it
to a concrete date.
This implies that you can't encode a duration into a long.
ps: your earlier comment about using an array of shorts to save memory
doesn't apply; bytes, shorts, characters are all stored as ints
internally.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@yahoo.com>.
--- On Mon, 2/15/10, Adrian Crum <ad...@yahoo.com> wrote:
> From: Adrian Crum <ad...@yahoo.com>
> Subject: Re: TimeDuration is not immutable
> To: dev@ofbiz.apache.org
> Date: Monday, February 15, 2010, 11:11 PM
> --- On Mon, 2/15/10, Adam Heath
> <do...@brainfood.com>
> wrote:
> > From: Adam Heath <do...@brainfood.com>
> > Subject: Re: TimeDuration is not immutable
> > To: dev@ofbiz.apache.org
> > Date: Monday, February 15, 2010, 10:55 PM
> > Adrian Crum wrote:
> > > Adam Heath wrote:
> > >> Adrian Crum wrote:
> > >>> Adam Heath wrote:
> > >>>> Adrian Crum wrote:
> > >>>>> Adam Heath wrote:
> > >>>>>> The javadoc for
> > org.ofbiz.base.util.TimeDuration says it is
> > >>>>>> immutable;
> > >>>>>> however, it's fields are not
> > final, and lots of methods modify it's
> > >>>>>> fields.
> > >>>>> The methods that modify fields
> are
> > directly or indirectly called by
> > >>>>> the
> > >>>>> constructor and are protected.
> Let me
> > know if any public method
> > >>>>> modifies
> > >>>>> a field.
> > >>>> fromLong modifies the fields
> directly, not
> > from the constructor.
> > >>> fromLong is a static method that returns
> an
> > immutable instance.
> > >>
> > >> Right you are, but the statement still
> > stands. fromLong should use
> > >> the constructor to set that.
> > >>
> > >>>> makeNegative modifies the fields.
> > >>> makeNegative is protected and used
> > internally.
> > >>
> > >> The the same statement for set() applies.
> > >>
> > >>>> set() is protected, TimeDuration is
> not
> > final, so sub-classes could
> > >>>> change the fields.
> > >>> If I need to extend TimeDuration, then I
> might
> > also need to change the
> > >>> fields.
> > >>
> > >> If fromLong just used the constructor, after
> doing
> > math on the long,
> > >> then it wouldn't need to call makeNegative
> > itself. Then, the only
> > >> places that would call set() or makeNegative
> would
> > be the constructor,
> > >> so the code could be inlined. Once it is
> > inlined, then the fields can
> > >> be made final, and the class becomes a
> proper
> > immutable.
> > >>
> > >> Additionally, becase fromLong creates an
> empty
> > TimeDuration, then sets
> > >> the fields one by one, the vm is free to
> re-order
> > those values, and
> > >> even delay then for a *long* time, well
> after
> > fromLong returns to it's
> > >> caller. There is no synchronization on
> this;
> > if these fields were
> > >> only set from the constructor, then this
> problem
> > wouldn't occur.
> > >>
> > >>>> Ideally, this class should extend
> Number,
> > or some other number class.
> > >>
> > >>> Why?
> > >> So we can do TimeDuration.longValue(), and
> get the
> > internal duration.
> > >
> > > What would it mean if you called
> > TimeDuration.intValue()? What would
> > > that value represent?
> > >
> > > In designing the API, I was trying to avoid
> equating a
> > duration with a
> > > long value. The current support for long values
> was
> > added to accommodate
> > > existing entity fields that store durations as
> > longs. The long value
> > > should not be confused with elapsed milliseconds
> - it
> > is a TimeDuration
> > > *encoded* as a long. (Another change I never got
> > around to was renaming
> > > the parameters so there is no reference to
> millis.)
> > >
> > > The whole idea of TimeDuration is that it is a
> data
> > type. It would be
> > > nice to be able to do math on TimeDurations (X
> > duration plus Y duration
> > > equals Z duration), but at the same time, you
> > shouldn't be able to do
> > > math on the internal representation of the
> duration
> > (since the
> > > implementation is supposed to be hidden).
> > >
> > >> Also, shouldn't the duration store the value
> > internally as a long?
> > >> That would solve the normalization problem,
> and
> > also reduce the memory
> > >> usage.
> > >
> > > I don't recall my reasoning for that. I guess it
> was
> > for performance
> > > reasons. If the internal value was a long, then
> each
> > accessor would have
> > > to decode the long.
> > >
> > >> I'm willing to do all these changes; first
> would
> > be to have test cases
> > >> and full coverage.
> > >
> > > That would be great! I created the class to make
> > calendaring code
> > > simpler. It could use some TLC.
> >
> > Ok, sorry to say this, but TimeDuration needs to be
> > removed.
> > com.ibm.icu.text.DurationFormat, which handles things
> like
> > 2 days from
> > now and 3 hours ago, uses a long for the duration.
> >
> > Additionally, trying to duplicate leap year
> calculation,
> > there is not
> > a whole number of days in a year(it's not quite
> > 365.24). If long is
> > good enough for icu, then why should we try to use
> > something else?
>
> Grasshopper,
>
> TimeDuration is not concerned with such details. They are
> delegated to the Calendar class.
>
> Tell me, using com.ibm.icu.text.DurationFormat - how do you
> add two durations?
Better yet, I depart LAX 23:00 Feb 15 2010 (Gregorian calendar) and arrive in India (whatever arrival time and calendar is in Hindi). What was my flight duration?
TimeDuration MUST be a data type. It doesn't work otherwise.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@yahoo.com>.
--- On Mon, 2/15/10, Adam Heath <do...@brainfood.com> wrote:
> From: Adam Heath <do...@brainfood.com>
> Subject: Re: TimeDuration is not immutable
> To: dev@ofbiz.apache.org
> Date: Monday, February 15, 2010, 10:55 PM
> Adrian Crum wrote:
> > Adam Heath wrote:
> >> Adrian Crum wrote:
> >>> Adam Heath wrote:
> >>>> Adrian Crum wrote:
> >>>>> Adam Heath wrote:
> >>>>>> The javadoc for
> org.ofbiz.base.util.TimeDuration says it is
> >>>>>> immutable;
> >>>>>> however, it's fields are not
> final, and lots of methods modify it's
> >>>>>> fields.
> >>>>> The methods that modify fields are
> directly or indirectly called by
> >>>>> the
> >>>>> constructor and are protected. Let me
> know if any public method
> >>>>> modifies
> >>>>> a field.
> >>>> fromLong modifies the fields directly, not
> from the constructor.
> >>> fromLong is a static method that returns an
> immutable instance.
> >>
> >> Right you are, but the statement still
> stands. fromLong should use
> >> the constructor to set that.
> >>
> >>>> makeNegative modifies the fields.
> >>> makeNegative is protected and used
> internally.
> >>
> >> The the same statement for set() applies.
> >>
> >>>> set() is protected, TimeDuration is not
> final, so sub-classes could
> >>>> change the fields.
> >>> If I need to extend TimeDuration, then I might
> also need to change the
> >>> fields.
> >>
> >> If fromLong just used the constructor, after doing
> math on the long,
> >> then it wouldn't need to call makeNegative
> itself. Then, the only
> >> places that would call set() or makeNegative would
> be the constructor,
> >> so the code could be inlined. Once it is
> inlined, then the fields can
> >> be made final, and the class becomes a proper
> immutable.
> >>
> >> Additionally, becase fromLong creates an empty
> TimeDuration, then sets
> >> the fields one by one, the vm is free to re-order
> those values, and
> >> even delay then for a *long* time, well after
> fromLong returns to it's
> >> caller. There is no synchronization on this;
> if these fields were
> >> only set from the constructor, then this problem
> wouldn't occur.
> >>
> >>>> Ideally, this class should extend Number,
> or some other number class.
> >>
> >>> Why?
> >> So we can do TimeDuration.longValue(), and get the
> internal duration.
> >
> > What would it mean if you called
> TimeDuration.intValue()? What would
> > that value represent?
> >
> > In designing the API, I was trying to avoid equating a
> duration with a
> > long value. The current support for long values was
> added to accommodate
> > existing entity fields that store durations as
> longs. The long value
> > should not be confused with elapsed milliseconds - it
> is a TimeDuration
> > *encoded* as a long. (Another change I never got
> around to was renaming
> > the parameters so there is no reference to millis.)
> >
> > The whole idea of TimeDuration is that it is a data
> type. It would be
> > nice to be able to do math on TimeDurations (X
> duration plus Y duration
> > equals Z duration), but at the same time, you
> shouldn't be able to do
> > math on the internal representation of the duration
> (since the
> > implementation is supposed to be hidden).
> >
> >> Also, shouldn't the duration store the value
> internally as a long?
> >> That would solve the normalization problem, and
> also reduce the memory
> >> usage.
> >
> > I don't recall my reasoning for that. I guess it was
> for performance
> > reasons. If the internal value was a long, then each
> accessor would have
> > to decode the long.
> >
> >> I'm willing to do all these changes; first would
> be to have test cases
> >> and full coverage.
> >
> > That would be great! I created the class to make
> calendaring code
> > simpler. It could use some TLC.
>
> Ok, sorry to say this, but TimeDuration needs to be
> removed.
> com.ibm.icu.text.DurationFormat, which handles things like
> 2 days from
> now and 3 hours ago, uses a long for the duration.
>
> Additionally, trying to duplicate leap year calculation,
> there is not
> a whole number of days in a year(it's not quite
> 365.24). If long is
> good enough for icu, then why should we try to use
> something else?
Grasshopper,
TimeDuration is not concerned with such details. They are delegated to the Calendar class.
Tell me, using com.ibm.icu.text.DurationFormat - how do you add two durations?
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> Adrian Crum wrote:
>>>>> Adam Heath wrote:
>>>>>> The javadoc for org.ofbiz.base.util.TimeDuration says it is
>>>>>> immutable;
>>>>>> however, it's fields are not final, and lots of methods modify it's
>>>>>> fields.
>>>>> The methods that modify fields are directly or indirectly called by
>>>>> the
>>>>> constructor and are protected. Let me know if any public method
>>>>> modifies
>>>>> a field.
>>>> fromLong modifies the fields directly, not from the constructor.
>>> fromLong is a static method that returns an immutable instance.
>>
>> Right you are, but the statement still stands. fromLong should use
>> the constructor to set that.
>>
>>>> makeNegative modifies the fields.
>>> makeNegative is protected and used internally.
>>
>> The the same statement for set() applies.
>>
>>>> set() is protected, TimeDuration is not final, so sub-classes could
>>>> change the fields.
>>> If I need to extend TimeDuration, then I might also need to change the
>>> fields.
>>
>> If fromLong just used the constructor, after doing math on the long,
>> then it wouldn't need to call makeNegative itself. Then, the only
>> places that would call set() or makeNegative would be the constructor,
>> so the code could be inlined. Once it is inlined, then the fields can
>> be made final, and the class becomes a proper immutable.
>>
>> Additionally, becase fromLong creates an empty TimeDuration, then sets
>> the fields one by one, the vm is free to re-order those values, and
>> even delay then for a *long* time, well after fromLong returns to it's
>> caller. There is no synchronization on this; if these fields were
>> only set from the constructor, then this problem wouldn't occur.
>>
>>>> Ideally, this class should extend Number, or some other number class.
>>
>>> Why?
>> So we can do TimeDuration.longValue(), and get the internal duration.
>
> What would it mean if you called TimeDuration.intValue()? What would
> that value represent?
>
> In designing the API, I was trying to avoid equating a duration with a
> long value. The current support for long values was added to accommodate
> existing entity fields that store durations as longs. The long value
> should not be confused with elapsed milliseconds - it is a TimeDuration
> *encoded* as a long. (Another change I never got around to was renaming
> the parameters so there is no reference to millis.)
>
> The whole idea of TimeDuration is that it is a data type. It would be
> nice to be able to do math on TimeDurations (X duration plus Y duration
> equals Z duration), but at the same time, you shouldn't be able to do
> math on the internal representation of the duration (since the
> implementation is supposed to be hidden).
>
>> Also, shouldn't the duration store the value internally as a long?
>> That would solve the normalization problem, and also reduce the memory
>> usage.
>
> I don't recall my reasoning for that. I guess it was for performance
> reasons. If the internal value was a long, then each accessor would have
> to decode the long.
>
>> I'm willing to do all these changes; first would be to have test cases
>> and full coverage.
>
> That would be great! I created the class to make calendaring code
> simpler. It could use some TLC.
Ok, sorry to say this, but TimeDuration needs to be removed.
com.ibm.icu.text.DurationFormat, which handles things like 2 days from
now and 3 hours ago, uses a long for the duration.
Additionally, trying to duplicate leap year calculation, there is not
a whole number of days in a year(it's not quite 365.24). If long is
good enough for icu, then why should we try to use something else?
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> Adam Heath wrote:
>>> Adrian Crum wrote:
>>>> Adam Heath wrote:
>>>>> The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
>>>>> however, it's fields are not final, and lots of methods modify it's
>>>>> fields.
>>>> The methods that modify fields are directly or indirectly called by the
>>>> constructor and are protected. Let me know if any public method modifies
>>>> a field.
>>> fromLong modifies the fields directly, not from the constructor.
>> fromLong is a static method that returns an immutable instance.
>
> Right you are, but the statement still stands. fromLong should use
> the constructor to set that.
>
>>> makeNegative modifies the fields.
>> makeNegative is protected and used internally.
>
> The the same statement for set() applies.
>
>>> set() is protected, TimeDuration is not final, so sub-classes could
>>> change the fields.
>> If I need to extend TimeDuration, then I might also need to change the
>> fields.
>
> If fromLong just used the constructor, after doing math on the long,
> then it wouldn't need to call makeNegative itself. Then, the only
> places that would call set() or makeNegative would be the constructor,
> so the code could be inlined. Once it is inlined, then the fields can
> be made final, and the class becomes a proper immutable.
>
> Additionally, becase fromLong creates an empty TimeDuration, then sets
> the fields one by one, the vm is free to re-order those values, and
> even delay then for a *long* time, well after fromLong returns to it's
> caller. There is no synchronization on this; if these fields were
> only set from the constructor, then this problem wouldn't occur.
>
>>> Ideally, this class should extend Number, or some other number class.
>
>> Why?
> So we can do TimeDuration.longValue(), and get the internal duration.
What would it mean if you called TimeDuration.intValue()? What would
that value represent?
In designing the API, I was trying to avoid equating a duration with a
long value. The current support for long values was added to accommodate
existing entity fields that store durations as longs. The long value
should not be confused with elapsed milliseconds - it is a TimeDuration
*encoded* as a long. (Another change I never got around to was renaming
the parameters so there is no reference to millis.)
The whole idea of TimeDuration is that it is a data type. It would be
nice to be able to do math on TimeDurations (X duration plus Y duration
equals Z duration), but at the same time, you shouldn't be able to do
math on the internal representation of the duration (since the
implementation is supposed to be hidden).
> Also, shouldn't the duration store the value internally as a long?
> That would solve the normalization problem, and also reduce the memory
> usage.
I don't recall my reasoning for that. I guess it was for performance
reasons. If the internal value was a long, then each accessor would have
to decode the long.
> I'm willing to do all these changes; first would be to have test cases
> and full coverage.
That would be great! I created the class to make calendaring code
simpler. It could use some TLC.
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> Adrian Crum wrote:
>>> Adam Heath wrote:
>>>> The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
>>>> however, it's fields are not final, and lots of methods modify it's
>>>> fields.
>>> The methods that modify fields are directly or indirectly called by the
>>> constructor and are protected. Let me know if any public method modifies
>>> a field.
>>
>> fromLong modifies the fields directly, not from the constructor.
>
> fromLong is a static method that returns an immutable instance.
Right you are, but the statement still stands. fromLong should use
the constructor to set that.
>
>> makeNegative modifies the fields.
>
> makeNegative is protected and used internally.
The the same statement for set() applies.
>> set() is protected, TimeDuration is not final, so sub-classes could
>> change the fields.
>
> If I need to extend TimeDuration, then I might also need to change the
> fields.
If fromLong just used the constructor, after doing math on the long,
then it wouldn't need to call makeNegative itself. Then, the only
places that would call set() or makeNegative would be the constructor,
so the code could be inlined. Once it is inlined, then the fields can
be made final, and the class becomes a proper immutable.
Additionally, becase fromLong creates an empty TimeDuration, then sets
the fields one by one, the vm is free to re-order those values, and
even delay then for a *long* time, well after fromLong returns to it's
caller. There is no synchronization on this; if these fields were
only set from the constructor, then this problem wouldn't occur.
>> Ideally, this class should extend Number, or some other number class.
So we can do TimeDuration.longValue(), and get the internal duration.
Also, shouldn't the duration store the value internally as a long?
That would solve the normalization problem, and also reduce the memory
usage.
> Why?
>
>> It should also normalize it's values. If I set a duration of 86400
>> seconds, it should normalize to 1 day.
>
> Agreed. That was something I always wanted to add, but never got around
> to it.
>
>> Also, the 7 arg constructor incorrectly sets itself negative. If I
>> set a duration of 5 days, and -30 seconds, it'll think it's negative.
>
> If normalization code was written, then that scenario could be
> accommodated. For now a duration is positive or negative.
I'm willing to do all these changes; first would be to have test cases
and full coverage.
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> Adrian Crum wrote:
>> Adam Heath wrote:
>>> The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
>>> however, it's fields are not final, and lots of methods modify it's
>>> fields.
>> The methods that modify fields are directly or indirectly called by the
>> constructor and are protected. Let me know if any public method modifies
>> a field.
>
> fromLong modifies the fields directly, not from the constructor.
fromLong is a static method that returns an immutable instance.
> makeNegative modifies the fields.
makeNegative is protected and used internally.
> set() is protected, TimeDuration is not final, so sub-classes could
> change the fields.
If I need to extend TimeDuration, then I might also need to change the
fields.
> Ideally, this class should extend Number, or some other number class.
Why?
> It should also normalize it's values. If I set a duration of 86400
> seconds, it should normalize to 1 day.
Agreed. That was something I always wanted to add, but never got around
to it.
> Also, the 7 arg constructor incorrectly sets itself negative. If I
> set a duration of 5 days, and -30 seconds, it'll think it's negative.
If normalization code was written, then that scenario could be
accommodated. For now a duration is positive or negative.
Re: TimeDuration is not immutable
Posted by Adam Heath <do...@brainfood.com>.
Adrian Crum wrote:
> Adam Heath wrote:
>> The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
>> however, it's fields are not final, and lots of methods modify it's
>> fields.
>
> The methods that modify fields are directly or indirectly called by the
> constructor and are protected. Let me know if any public method modifies
> a field.
fromLong modifies the fields directly, not from the constructor.
makeNegative modifies the fields.
set() is protected, TimeDuration is not final, so sub-classes could
change the fields.
If this were a proper immutable class, it could be used as a key in a map.
Ideally, this class should extend Number, or some other number class.
It should also normalize it's values. If I set a duration of 86400
seconds, it should normalize to 1 day.
Also, the 7 arg constructor incorrectly sets itself negative. If I
set a duration of 5 days, and -30 seconds, it'll think it's negative.
>> Was there any research into other duration implementations? I see one
>> at javax.xml.datatype.Duration, that seems mostly compatible, except
>> that it doesn't have millisecond resolution.
>
> I spent some time Googling publicly available JavaDocs and got the basic
> idea from what I found.
>
Re: TimeDuration is not immutable
Posted by Adrian Crum <ad...@hlmksw.com>.
Adam Heath wrote:
> The javadoc for org.ofbiz.base.util.TimeDuration says it is immutable;
> however, it's fields are not final, and lots of methods modify it's
> fields.
The methods that modify fields are directly or indirectly called by the
constructor and are protected. Let me know if any public method modifies
a field.
> Was there any research into other duration implementations? I see one
> at javax.xml.datatype.Duration, that seems mostly compatible, except
> that it doesn't have millisecond resolution.
I spent some time Googling publicly available JavaDocs and got the basic
idea from what I found.