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