Re: RFR 8191216: SimpleTimeZone.clone() has a data race on cache fields

[Peter Levart]

Hi Naoto,

Thank you for reviewing.

Naoto Sato je 06. 12. 2017 ob 20:41 napisal:

Hi Peter, Venkat,

Thank you for the fix. It looks good to me. Improved performance is a 
nice bonus! Would you be able to provide with a regression test?


Sure, here it is:

http://cr.openjdk.java.net/~plevart/jdk10-dev/8191216_SimpleTimeZone_clone_race/webrev.02/


The test reliably detects several races in 2 seconds of runtime which 
cause incorrect offset calculations in JDK 9:


shared TZ: 7363986 correct, 0 incorrect calculations
cloned TZ: 3960264 correct, 1180 incorrect calculations
Exception in thread "main" java.lang.AssertionError: 1180 fatal data 
races detected
    at 
SimpleTimeZoneCloneRaceTest.main(SimpleTimeZoneCloneRaceTest.java:86)


With patch applied, there are no failures of course.

Can this go into JDK 10 ?

Regards, Peter



Naoto

On 12/6/17 6:10 AM, Peter Levart wrote:

Hi,

On 12/06/2017 02:30 PM, Alan Bateman wrote:
I think this class is normally maintained on i18n-dev but I think 
introducing the Cache object looks good and making this much easier 
to understand.


-Alan 


Thanks Alan, I'm forwarding to i18n-dev to see if maintainers of that 
part of JDK have any comments...


This is an official request for reviewing a patch for fixing a data 
race between cloning a SimpleTimeZone object and lazily initializing 
its 3 cache fields which may produce a clone with inconsistent cache 
state. Here's Jira issue with details:


https://bugs.openjdk.java.net/browse/JDK-8191216

Venkat has proposed to simply call invalidateCache() on the clone 
before returning it from SimpleTimeZone.clone() method:


 public Object clone()
 {
 SimpleTimeZone clone = (SimpleTimeZone) super.clone();
 clone.invalidateCache();
 return clone;
 }

This fixes the issue and for the case of TimeZone.getDefault() which 
is called from ZoneId.systemDefault() even elides synchronization in 
clone.invalidateCache() and allocation of the clone, so JITed 
ZoneId.systemDefault() is unaffected by the patch. Initially I was 
satisfied with the fix, but then I tested something. Suppose someone 
sets default zone to SimpleTimeZone:


 TimeZone.setDefault(
 new SimpleTimeZone(360,
    "Europe/Paris",
    Calendar.MARCH, -1, Calendar.SUNDAY,
    360, SimpleTimeZone.UTC_TIME,
    Calendar.OCTOBER, -1, Calendar.SUNDAY,
    360, SimpleTimeZone.UTC_TIME,
    360)
 );

And then calls some methods that initialize the cache of the internal 
shared TimeZone.defaultTimeZone instance:


 new Date().toString();

The code which after that tries to obtain default time zone and 
calculate the offset from UTC at some current point in time, for 
example:


 TimeZone.getDefault().getOffset(now)

can't use the cached state because it has been invalidated in the 
returned clone. Such code has to re-compute the offset every time it 
gets new clone. I measured this with a JMH benchmark and got the 
following result:


Default:

Benchmark  Mode  Cnt Score Error  Units
ZoneIdBench.TimeZone_getDefault_getOffset  avgt   10 57,168 ± 0,501  
ns/op
ZoneIdBench.ZoneId_systemDefault   avgt   10 3,558 ± 0,040  
ns/op


Venkat's patch - invalidateCache():

Benchmark  Mode  Cnt Score Error  Units
ZoneIdBench.TimeZone_getDefault_getOffset  avgt   10 179,476 ± 1,942  
ns/op
ZoneIdBench.ZoneId_systemDefault   avgt   10 3,538 ± 0,073  
ns/op


We see that ZoneId.systemDefault() is unaffected, but 
TimeZone.getDefault().getOffset(now) becomes 3x slower.


This is not good, so I tried an alternative fix for the issue - 
simply making the SimpleTimeZone.clone() synchronized. Access to 
cache fields is already synchronized, so this should fix the race. 
Here's the JMH result:


Default:

Benchmark  Mode  Cnt Score Error  Units
ZoneIdBench.TimeZone_getDefault_getOffset  avgt   10 57,168 ± 0,501  
ns/op
ZoneIdBench.ZoneId_systemDefault   avgt   10 3,558 ± 0,040  
ns/op


Synchronized clone():

Benchmark  Mode  Cnt Score Error  Units
ZoneIdBench.TimeZone_getDefault_getOffset  avgt   10 58,103 ± 0,936  
ns/op
ZoneIdBench.ZoneId_systemDefault   avgt   10 4,154 ± 0,034  
ns/op


We see that caching works again, but synchronization has some 
overhead which is not big for single-threaded access, but might get 
bigger when multiple threads try to get default zone concurrently.


So I created a 3rd variant of the fix which I'm presenting here and 
requesting a review for:


http://cr.openjdk.java.net/~plevart/jdk10-dev/8191216_SimpleTimeZone_clone_race/webrev.01/ 



The JMH benchmark shows this:

Default:

Benchmark  Mode  Cnt Score Error  Units
Z

Re: RFR(s): 8177681: Remove methods Runtime.getLocalized{Input, Output}Stream

[Martijn Verburg]

That must be oddly satisfying :-D

Cheers,
Martijn

On 7 December 2017 at 03:35,  wrote:

> 2017/12/6 17:33:36 -0500, stuart.ma...@oracle.com:
> > Please review the removal of these methods, which were part of an
> obsolete
> > internationalization mechanism. They were deprecated in JDK 1.1 and
> deprecated
> > for removal in JDK 9. As far as I can see, there is no usage of these
> methods
> > anywhere.
>
> As the developer responsible for deprecating these methods
> in JDK 1.1 -- way back in 1996 --- I heartily approve of this
> change!
>
> - Mark
>

Re: RFR 4358774: Add null InputStream and OutputStream

[Patrick Reinhart]

Hi Brian,

> All previous suggested changes have been made in
> 
> http://cr.openjdk.java.net/~bpb/4358774/webrev.03/
> 

  99 @Override
 100 public int read(byte[] b, int off, int len) throws IOException 
{
 101 Objects.checkFromIndexSize(off, len, b.length);

Should not be checked for a null byte buffer b here?

-Patrick

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Peter Levart]

Peter Levart je 09. 12. 2017 ob 21:01 napisal:

Hi Claes,

Claes Redestad je 09. 12. 2017 ob 03:19 napisal:

Hi John,

On 2017-12-09 02:20, John Rose wrote:
On Dec 8, 2017, at 4:45 PM, John Rose > wrote:


Can anyone point out a reason why the value based
lists of List.of() should serialize while the value based
lists of List.of().subList() should not?  Or is there some
reason we should not allow subList to produce value
based lists?


One thing that might be implied from the specification that talks 
about "...a view of the portion of this list between the specified 
indexes..." is that the view keeps a reference to the original list. 
This is observable if combined with Reference object(s). But I don't 
know if this is an important behavioral detail that any code depends on.


Regards, Peter


BTW, ImmutableCollections.AbstractImmutableList.SubList already violates 
that assumption and nobody is complaining.


Peter

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Peter Levart]

Hi Claes,

Claes Redestad je 09. 12. 2017 ob 03:19 napisal:

Hi John,

On 2017-12-09 02:20, John Rose wrote:
On Dec 8, 2017, at 4:45 PM, John Rose > wrote:


Can anyone point out a reason why the value based
lists of List.of() should serialize while the value based
lists of List.of().subList() should not?  Or is there some
reason we should not allow subList to produce value
based lists?


One thing that might be implied from the specification that talks about 
"...a view of the portion of this list between the specified indexes..." 
is that the view keeps a reference to the original list. This is 
observable if combined with Reference object(s). But I don't know if 
this is an important behavioral detail that any code depends on.


Regards, Peter

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Andrej Golovnin]

Hi Claes,

> For example, returning Spliterators.emptySpliterator() and
> Collections.singletonSpliterator when appropriate *sounds* like nice little
> optimizations, but Arrays.spliterator(T[]) with an empty or single-element 
> array
> appears to be relatively cheap operations, whereas mixing implementation could
> make call-sites accepting List.of(foo).spliterator() become megamorphic.
> 
> Thus I think these should be done independently as a follow-up, along with
> tests and microbenchmarks.

I’m fine with it. The most important part is that you now aware of this problem
and I’m sure that you would provide the best possible solution. :-)

Best regards,
Andrej Golovnin

Re: Add EnumMap.keyType() and EnumSet.elementType()

[Andrej Golovnin]

Hi Rémi,

> It's like exposing reified type arguments, i can see these kind of change 
> back firing in at least two ways.
> It's not future proof, part of valhalla requires to change generics to allow 
> generics of value types which requires a form of reification, but we may end 
> up with a light form of reification similar to Scala type manifest (type 
> arguments are available at construction time but not after that point) 
> exposing type arguments for EnumSet will be seen as a mismatch in that case. 
> Also, we may want to introduce unmodifiable EnumSet in the future, currently 
> an unmodifiable empty EnumSet do not need to store any type argument, the 
> proposed API force us to have as many empty EnumSet as type arguments 
> combination (it's why you have one static field by type specialization in 
> C#), again it makes the proposed API not future proof.

Only the death is future proof. Everything else is debatable. (c) :-P
Empty EnumSet is not a problem. As I already said there is a workaround
(an ugly one, but it is there). The real problem is an empty EnumMap.
When the valhalla project would provide a solution to my problem, then
I’m fine with it. When not, then let us debate about an alternative solution. 
:-)

Best regards,
Andrej Golovnin 

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Claes Redestad]

Hi Andrej,

forEach seems like a no-brainer, but spliterator might require some 
extra care.


For example, returning Spliterators.emptySpliterator() and
Collections.singletonSpliterator when appropriate *sounds* like nice little
optimizations, but Arrays.spliterator(T[]) with an empty or 
single-element array
appears to be relatively cheap operations, whereas mixing implementation 
could

make call-sites accepting List.of(foo).spliterator() become megamorphic.

Thus I think these should be done independently as a follow-up, along with
tests and microbenchmarks.

Thanks!

/Claes

On 2017-12-09 11:43, Andrej Golovnin wrote:

Hi Claes,


One more thing: Could you please add specialised implementations also
for the following methods:

List.forEach(Consumer)

List.spliterator()
  For List12 when List12.size() == 1 please use 
Collections.singletonSpliterator()
  (this method should be moved to the Spliterators class and be public).
  For List12 when List12.size() == 2 please use Arrays.spliterator().

  For ListN when List.isEmpty() == true please use 
Spliterators.emptySpliterator​()
  and otherwise use Arrays.spliterator().


I’m sorry I forgot to mention, that Set12, SetN, Map12 and MapN should also
have specialised implementations for the #forEach-methods and for
the #spliterator()-methods in Set12, SetN.

Thanks!

Best regards,
Andrej Golovnin

Re: Add EnumMap.keyType() and EnumSet.elementType()

[Remi Forax]

Hi Andrej,
I do not think it's a good idea.

It's like exposing reified type arguments, i can see these kind of change back 
firing in at least two ways.
It's not future proof, part of valhalla requires to change generics to allow 
generics of value types which requires a form of reification, but we may end up 
with a light form of reification similar to Scala type manifest (type arguments 
are available at construction time but not after that point) exposing type 
arguments for EnumSet will be seen as a mismatch in that case. Also, we may 
want to introduce unmodifiable EnumSet in the future, currently an unmodifiable 
empty EnumSet do not need to store any type argument, the proposed API force us 
to have as many empty EnumSet as type arguments combination (it's why you have 
one static field by type specialization in C#), again it makes the proposed API 
not future proof.

regards,
Rémi 

- Mail original -
> De: "Stephen Colebourne" 
> À: "core-libs-dev" 
> Envoyé: Jeudi 7 Décembre 2017 13:09:40
> Objet: Re: Add EnumMap.keyType() and EnumSet.elementType()

> I'm surprised I've never run into this. This seems like a simple and
> useful change.
> Stephen
> 
> On 7 December 2017 at 11:40, Andrej Golovnin  
> wrote:
>> Hi all,
>>
>> it would be nice if we would have access to the class of the enum type
>> used to create an EnumMap or an EnumSet.
>>
>> This is usefull when you write a custom serialization library and
>> would like to serialize/deserialize an empty EnumMap or an empty
>> EnumSet. For the empty EnumSet there is a workaround to get the enum
>> class:
>>
>> EnumSet empty = EnumSet.noneOf(MyEnum.class);
>> EnumSet tmp = EnumSet.complementOf(empty);
>> Class elementType = tmp.iterator().next().getClass();
>>
>> But this only works when the enum class has at least one enum. For
>> EnumMap there is no such workaround at all and we have to use the
>> Reflection API. And you know the warnings since Java 9 :-) :
>>
>> WARNING: An illegal reflective access operation has occurred
>> WARNING: Illegal reflective access by c.r.r.k.EnumMapSerializer to
>> field java.util.EnumMap.keyType
>> WARNING: Please consider reporting this to the maintainers of
>> c.r.r.k.EnumMapSerializer
>> WARNING: Use --illegal-access=warn to enable warnings of further
>> illegal reflective access operations
>> WARNING: All illegal access operations will be denied in a future release
>>
>> So to avoid this warning and to avoid that our application stops to
>> work with a future release of Java I would like to propose to add the
>> following two methods:
>>
>> EnumMap:
>> /**
>>  * Returns the {@code Class} object of the key type for this enum map.
>>  *
>>  * @return the {@code Class} object of the key type for this enum map.
>>  *
>>  * @since 10
>>  */
>> public Class keyType()
>>
>>
>> EnumSet:
>> /**
>>  * Returns the {@code Class} object of all the elements of this set.
>>  *
>>  * @return the {@code Class} object of all the elements of this set.
>>  *
>>  * @since 10
>>  */
>> public Class elementType()
>>
>> The suggested change is attached as diff.
>>
>> Best reagrds,
> > Andrej Golovnin

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Andrej Golovnin]

Hi Claes,

> One more thing: Could you please add specialised implementations also
> for the following methods:
> 
> List.forEach(Consumer)
> 
> List.spliterator()
>  For List12 when List12.size() == 1 please use 
> Collections.singletonSpliterator()
>  (this method should be moved to the Spliterators class and be public).
>  For List12 when List12.size() == 2 please use Arrays.spliterator().
> 
>  For ListN when List.isEmpty() == true please use 
> Spliterators.emptySpliterator​()
>  and otherwise use Arrays.spliterator().
> 

I’m sorry I forgot to mention, that Set12, SetN, Map12 and MapN should also
have specialised implementations for the #forEach-methods and for
the #spliterator()-methods in Set12, SetN.

Thanks!

Best regards,
Andrej Golovnin

Re: [10?] RFR: 8193128: Reduce number of implementation classes returned by List/Set/Map.of()

[Andrej Golovnin]

Hi Claes,

> I think one can interpret the @implSpec in AbstracList::subList to suggest
> that the implementation retrieved will subclass AbstractList, which implies
> it's *not* Serializable.

Not for me. java.util.ArrayList is a subclass of AbstractList and is 
serializable.
And you can use j.u.ArrayList to implement the #subList-method in a subclass of
AbstractList without violating the @implSpec in AbstractList#subList.

And as John already mentioned AbstractList is not a part of the Public API of
the lists returned by the List#of()-methods. So the only spec you should care
about is the one defined in List#subList(). And this spec says nothing about
whether the returned instance should be searializable or not.

> As time is likely up for getting this into 10 then I guess we might as well
> take a step back and do this right for 11. I suspect we'll need a CSR for
> this RFE regardless.
> 
> Keeping it all down to two classes as you suggest allows any mixed usage to
> stay in the realm of bimorphism which might bring a considerable speedup
> in some cases, and the reduction in number of implementation classes
> loaded is a boon. Having all methods in ListN account for the offset might
> cost us a few percent here and there, though.
> 
> An int offset on ListN would actually be footprint neutral compared to the
> pre-existing implementation which pulls in the int modCount from AbstractList.

I really like the idea from John to use List12 and ListN for sublists as it 
would give
us for free the fast implementations for the methods like indexOf, lastIndexOf, 
etc. even
for sublists. But I would not use offset. Do you remember the problem with 
String#substring()
when the #substring()-method has returned a view over very huge String object
which then could not be garbage collected due to a reference from the substring?

I would just add a new constructor to ListN:

ListN(E[] elements, int fromIndex, int toIndex)

to copy elements from the parent list. And this would allow us to keep 
the implementation of the ListN class as is.

One more thing: Could you please add specialised implementations also
for the following methods:

List.forEach(Consumer)

List.spliterator()
  For List12 when List12.size() == 1 please use 
Collections.singletonSpliterator()
  (this method should be moved to the Spliterators class and be public).
  For List12 when List12.size() == 2 please use Arrays.spliterator().
 
  For ListN when List.isEmpty() == true please use 
Spliterators.emptySpliterator​()
  and otherwise use Arrays.spliterator().


> http://cr.openjdk.java.net/~redestad/8193128/open.04/

In case you still would like to use the SubList class:

115 int size;

The size field should be final.

Thanks!

Best regards,
Andrej Golovnin

Re: RFR(s): 8140281 add no-arg Optional.orElseThrow() as preferred alternative to get()

[Stephen Colebourne]

On 8 December 2017 at 00:33, Stuart Marks  wrote:
> Please review this changeset that introduces a new no-arg method
> orElseThrow() to Optional, as a preferred alternative to the get() method.

I am willing to accept this addition as it has an obvious parallel to
the existing supplier method.

My position on removing get() has not changed however, and as such I
am displeased with the addition of the "preferred alternative" text.

Stephen