from:"stuart . marks"

Re: Some Classes with a public void close() don't implement AutoCloseable

2020-04-21 Thread Stuart Marks


Hi Johannes,

On 4/16/20 3:09 AM, Johannes Kuhn wrote:

About java.xml.stream: A XMLStreamReader is for example constructed with:

     BufferedReader br = ...;
     XMLStreamReader xsr = 
XMLInputFactory.newDefaultFactory().createXMLEventReader(br);


For my IDE this would look like XMLStreamReader wraps the BufferedReader, so it 
won't issue a warning if I don't close br.

But the Javadoc for XMLStreamReader.close() states:

 > Frees any resources associated with this Reader. This method does not close 
the underlying input source.


The typical try-with-resources idiom for this looks like:

try (var br = new BufferedReader(...);
 var xsr = 
XMLInputFactory.newDefaultFactory().createXMLEventReader(br)) {
...
}

This should work even though XMLEventReader.close() is specified not to close 
the underlying Reader. It will also work to close the BufferedReader if the 
creation of XMLEventReader throws an exception.


(The usual objection to this construct is if closing the wrapper closes the 
underlying reader, the t-w-r will close it again. This isn't a problem for 
BufferedReader and most JDK I/O classes, as close() is idempotent for them.)



If those classes would implement AutoCloseable, people and IDEs might wrongly 
assume that it would close the BufferedReader.
Which would result in code like this:

try (var xsr = 
XMLInputFactory.newDefaultFactory().createXMLEventReader(Files.newBufferedReader(path,
 StandardCharsets.UTF_8))) {  ... }

And have a resource leak. Incidentally, that is the similar to the code I tried 
to write when I saw that XMLStreamReader has a public void close() method.


I'm not sure what to make of what an IDE tells you. If it's making unwarranted 
assumptions about whether XMLEventReader (or some other wrapper) does or does 
not close the wrapped object, then it's a bug.


Note that the nested construction example shown here

try (var xsr = createXMLEventReader(Files.newBufferedReader(...)))

*will* leak the BufferedReader if createXMLEventReader() throws an exception, 
since nobody has kept the reference to the BufferedReader. So in all cases, I 
recommend that people use the multiple variable declaration form of t-w-r.



So in my opinion, to let XMLStreamReader implement AutoCloseable, the 
specification of the close() method should be changed.


This would be an incompatible change. There might be code that relies on 
XMLStreamReader not to close the underlying reader, and reuses the reader for 
something.



The same applies for the other XML Stream/Event Reader/Writer.


On the face of it, making these AutoCloseable seems reasonable to me. However, 
I'm somewhat distant from this area, so I don't know if that makes sense for 
these particular XML interfaces.


s'marks



Thanks,
Johannes

On 16-Apr-20 9:28, Alan Bateman wrote:

On 16/04/2020 01:28, Johannes Kuhn wrote:

:

I did not restrict my enumeration to public and exported types - it was 
easier not to do that and I'm lazy.
Most of the candidates that you've identified are JDK internal classes and 
there are several non-public classes in exported packages. There are also a 
few cases where the sub-classes of the likely candidate are showing up too 
(e.g. all the public sub-classes of java.util.logging.Handler). I skimmed 
through your list and filtered it down to the public classes in exported 
packages to following:


com/sun/jdi/connect/spi/Connection
java/awt/SplashScreen
java/util/logging/Handler
javax/naming/Context
javax/naming/NamingEnumeration
javax/naming/ldap/StartTlsResponse
javax/smartcardio/CardChannel
javax/sql/PooledConnection
javax/swing/ProgressMonitor
javax/xml/stream/XMLEventReader
javax/xml/stream/XMLEventWriter
javax/xml/stream/XMLStreamReader
javax/xml/stream/XMLStreamWriter

As Joe points out, some of these may have been looked at already. I was 
surprised to see the java.xml.stream reader/writers so it might be worth 
digging into those to see why they were not retrofitted.


-Alan.

Re: Some Classes with a public void close() don't implement AutoCloseable

2020-04-22 Thread Stuart Marks


On 4/22/20 1:42 PM, Joe Darcy wrote:

On 4/22/2020 6:12 AM, Alan Bateman wrote:

On 22/04/2020 13:50, Andrew Haley wrote:

:
1. Should close() always be idempotent, where practical? I would have
    thought so, but perhaps there are downsides.

2. Should classes which implement close() with the standard meaning be
    AutoCloseable?

I'm sure Joe Darcy can say more on this but I remember there was a lot of 
effort put into this topic when AutoCloseable was added in Java 7 (and Project 
Coin). Closeable close is idempotent but AutoCloseable close could not require 
it. AutoCloseable's API docs recommend it of course. There was effort in Java 
7 and beyond to retrofit existing classes that defined a close method to be 
Closeable or AutoCloseable. There are only a handful of exported APIs 
remaining that have a close method that don't extend or implement 
AutoCloseable. I don't know the history of the XML stream interface to know 
why they close to define them not to close the underlying stream but I doubt 
these could be changed now.


Yes, the JSR 334 EG had some discussions about both "SilentCloseable" (no 
exceptions) and "IdempotentCloseable" as possible library additions back in the 
JDK 7 time frame. These were not judged to have sufficient marginal utility over 
Closeable and AutoCloseable to include in the platform.


It was impractical to require idempotent close methods in call cases, but they 
are recommended. Generally, a type with a void close method should implement 
AutoClosable. Most of the candidate types in the JDK were updated for JDK 7; a 
few stragglers were updated since then, but there are a few remaining cases that 
could be updated as discussed earlier in this thread.


Regarding idempotentcy, I remember some of those discussions. To the extent 
possible, JDK implementations all have idempotent close() methods. However, some 
classes that seemed useful to be AutoCloseable were extensible outside the JDK, 
and we couldn't guarantee the idempotency of those close() implementations. It 
seemed reasonable to make the JDK classes AutoCloseable but for AC not to 
require idempotency.


To answer Andrew's second question, I think whether something ought to implement 
AC this depends on the "standard meaning" of close(), but the answer is likely 
yes. I think the "standard meaning" includes some ideas such as the possibility 
of the object referencing some resource external to the JVM, not under (direct) 
control of the garbage collector, for which it would be considered a resource 
leak for close() not to be called, and for which prompt release of that resource 
is considered important.


There's another small wrinkle with AutoCloseable which is that its definition 
changed somewhat in Java 8. Prior to Java 8 there was a sense that any AC 
instance ought to be used within a try-with-resources statement, and not doing 
so merited a warning. In Java 8 this was relaxed somewhat, in that T-W-R should 
be used only for AC instances that are known to contain external resources. The 
driver here was Stream, which implements AC. A Stream might represent a resource 
-- see Files.lines() for example -- in which case T-W-R should be used. However, 
many streams represent only in-memory values, so in those cases T-W-R is 
superfluous.


Finally, several years back there was a bit of discussion on core-libs-dev on 
the issue of whether the XML streams should implement AutoCloseable; see


http://mail.openjdk.java.net/pipermail/core-libs-dev/2013-May/017343.html

At the time the main issue seemed to be a complication with changing the 
specifications, because these APIs were also under the control of an independent 
(non Java SE platform) JSR. In fact that might have been the original reason for 
these APIs not to have been retrofitted in Java 7. Given the passage of time, 
perhaps this is no longer an issue.


s'marks

Re: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

2020-04-27 Thread Stuart Marks


Hi Tagir,

A few quick thoughts on this.

There does seem to be a conceptual hole here. Most collections implementations 
have an obvious interface that provides a reasonable abstraction of that 
implementation. However, one is "missing" for LinkedHashSet, since there's no 
interface more specialized than Set. This leaves it undifferentiated from 
HashSet. This proposal seems to fill that gap.


I'm having trouble seeing where this would be used in an API, though. A method 
could consume an OrderedSet, to prevent somebody from accidentally providing an 
unordered set. An OrderedSet as a return type would be good documentation that 
the returned set has an order, but this is pretty thin.


One point to consider is: not everything needs to be in the type system. Streams 
have a runtime flag that indicates ordering; there's no OrderedStream or 
anything like that. It seems to be sufficient.


There's also kind of a clash with SortedSet. Conceptually, SortedSet is an 
OrderedSet, but this would force some additional methods into SortedSet (or it 
would limit the methods provided by this one, possibly making it not very useful).


Setting aside the types, the added functionality of getting the last element and 
iterating in the reverse order seems somewhat useful. Does this justify a whole 
new interface?


This boils down to the questions,

 - Do we need a new type that indicates that the set has an order?
 - Or are we more interested in the added functionality?

This tells me that we should do some thinking about use cases. There was some 
Twitter chatter on this topic the other day that I suspect might have led to 
this proposal. (The tweets were a bit scattered, as it typical for Twitter, so 
I'm not sure it's worth linking to them.) In any case a couple things seemed to 
come out of it, namely, the ability to iterate a LHS in reverse, or possibly to 
get its last element. As I recall, you even mused about a Deque view of an LHS. 
(The methods proposed here seem well aligned with Deque.) It seems like this 
approach could be useful, and it's much lighter weight than introducing a new type.


Could you come up with some use cases and see whether and how they could be 
satisfied by different approaches?


s'marks




On 4/25/20 9:51 AM, Tagir Valeev wrote:

Hello!

To me, it was always a pity that the internal structure of
LinkedHashMap allows doing more things easily, yet this functionality
is not exposed to the external clients, so if somebody really needs
such things they will need to waste memory/CPU to emulate them. The
functionality I'm speaking about includes:
- getting the last (i.e. most recently added/accessed) entry
- getting the descending iterator going from newest to oldest entries
- getting the iterator that starts at given existing entry

It's easy to do such things with TreeMap and other NavigableMap
implementations. However, LinkedHashMap provides no additional visible
methods in addition to the Map interface (well, except
removeEldestEntry()). The same considerations apply to LinkedHashSet.

So we can provide new interfaces j.u.OrderedMap and OrderedSet that
provide the additional functionality and make
LinkedHashMap/LinkedHashSet implement them. Aside from new
functionality, the interfaces will carry additional semantics: their
iteration order is always predictable and their spliterators return
DISTINCT|ORDERED characteristics, so some clients may require
OrderedMap/Set just to assert that they rely on predictable iteration
order. We can even make existing NavigableMap/Set extend
OrderedMap/Set because NavigableMap/Set are ordered. Some of
NavigableMap/Set methods will naturally fit the OrderedMap/Set
interfaces.

I think I can devote my spare time to write the spec, implementation,
and tests for this enhancement, participate in the API design
discussions and do any other job necessary to make this improvement
happen. However, I'd like to know in advance whether such kind of
change would be met positively. What do you think? Is it possible to
do this or this improvement contradicts the OpenJDK goals and will
likely be rejected? Also, do I need to write a JEP for this?

It's probably too early to discuss the exact API, but for more
specificity of my proposal, here's the quick draft:

package java.util;

public interface OrderedSet extends Set {
   // First or NSEE if empty
   E first() throws NoSuchElementException;
   // Last or NSEE if empty
   E last() throws NoSuchElementException;
   // Retrieve&remove first or null if empty
   E pollFirst();
   // Retrieve&remove last or null if empty
   E pollLast();
   // Iterator starting from element, or NSEE if not found
   Iterator iteratorFrom(E fromElement) throws NoSuchElementException;
   // Descending
   Iterator descendingIterator();
   // Descending iterator starting from element, or NSEE if not found
   Iterator descendingIteratorFrom(E fromElement) throws
NoSuchElementException;
   // TBD: descendingSet()
}

All the methods except iter

Re: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

2020-04-28 Thread Stuart Marks


Hi Paul,

I too hesitate about adding Ordered* interfaces. As I said previously, I don't 
think they're very useful, and they do seem rather Queue- or Deque-like.


Indeed, Tagir was musing the other day on Twitter about a Deque view of 
LinkedHashSet and possibly LinkedHashMap. I think that might be a more 
worthwhile direction to pursue. Providing Deque views is more in keeping with 
the current collections idioms than making LHS/LHM implement Deque directly. 
This is rather like Map providing entry and key Set views and values Collection 
views.


Note that these "view collections" provide live views of the backing collection 
and that they aren't necessarily read-only. They support iteration in various 
forms (e.g., streaming), and they can also permit removal (but not addition). 
This is a bit odd but it can come in handy.


I'm thinking that the form of Iterable that supports access to first/last 
elements, and iteration in either order, is in fact Deque itself. Deque already 
supports descendingIterator(), which is a useful mechanism, but it's somewhat 
inconvenient: you can't use it with enhanced-for, stream, or toArray. It would 
be good to have a reversed-view Deque that flips the order of all the 
operations. That avoids having to provide descendingStream(), 
descendingToArray(), etc., and it adapts to anything that consumes an Iterable 
or Collection.


(It occurs to me that we're missing a Collections.unmodifiableDeque wrapper.)

The LHS/LHM views, and various Deque wrappers, should all be pretty simple, with 
the typical bunch of one-liner methods. (With the possible exception of 
toArray(T[]).) Whether they are sufficiently useful, though, is still an open 
question.


s'marks

On 4/28/20 10:59 AM, Paul Sandoz wrote:

Hi Tagir,

I am hesitant to add new interfaces specific to Set/Map for a non-indexed 
encounter order.

The interface OrderedSet is tantalizingly queue-like, in fact rather close to 
the read-only part of Deque.  Did you consider the possibility of LinkedHashSet 
implementing Deque?

I have not thought about this too deeply, but since LinkedHashMap is 
implemented as a doubly linked list it might be possible (as LinkedList 
implements Deque).

If so we could also add the method:

   LinkedHashSet> LinkedHashMap.entryLinkedSet()

and thereby avoid the addition of any new interfaces, although it might require 
some refinement to the specification of Deque; a new notion of an unmodifiable 
queue, which I admit is a little odd, and as a result is probably not be a good 
idea.

More generally I think the core abstraction is really a new form Iterable 
supporting access to first + last elements and reverse iteration (and possibly 
iteration starting from a containing element).

Paul.


On Apr 25, 2020, at 9:51 AM, Tagir Valeev  wrote:

Hello!

To me, it was always a pity that the internal structure of
LinkedHashMap allows doing more things easily, yet this functionality
is not exposed to the external clients, so if somebody really needs
such things they will need to waste memory/CPU to emulate them. The
functionality I'm speaking about includes:
- getting the last (i.e. most recently added/accessed) entry
- getting the descending iterator going from newest to oldest entries
- getting the iterator that starts at given existing entry

It's easy to do such things with TreeMap and other NavigableMap
implementations. However, LinkedHashMap provides no additional visible
methods in addition to the Map interface (well, except
removeEldestEntry()). The same considerations apply to LinkedHashSet.

So we can provide new interfaces j.u.OrderedMap and OrderedSet that
provide the additional functionality and make
LinkedHashMap/LinkedHashSet implement them. Aside from new
functionality, the interfaces will carry additional semantics: their
iteration order is always predictable and their spliterators return
DISTINCT|ORDERED characteristics, so some clients may require
OrderedMap/Set just to assert that they rely on predictable iteration
order. We can even make existing NavigableMap/Set extend
OrderedMap/Set because NavigableMap/Set are ordered. Some of
NavigableMap/Set methods will naturally fit the OrderedMap/Set
interfaces.

I think I can devote my spare time to write the spec, implementation,
and tests for this enhancement, participate in the API design
discussions and do any other job necessary to make this improvement
happen. However, I'd like to know in advance whether such kind of
change would be met positively. What do you think? Is it possible to
do this or this improvement contradicts the OpenJDK goals and will
likely be rejected? Also, do I need to write a JEP for this?

It's probably too early to discuss the exact API, but for more
specificity of my proposal, here's the quick draft:

package java.util;

public interface OrderedSet extends Set {
  // First or NSEE if empty
  E first() throws NoSuchElementException;
  // Last or NSEE if empty
  E last() throws NoSuchElementException;
  /

Re: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

2020-04-29 Thread Stuart Marks





On 4/28/20 9:48 PM, Jason Mehrens wrote:

Looks like It is intentional that unmodifiable queues are not present.  See: 
https://bugs.openjdk.java.net/browse/JDK-5030930.  The same logic would have 
been used for when Deque was added in the following release.


Good find.

Looking at the Queue interface, it adds four mutator methods and two access 
methods: element() and peek(). These latter two provide only a tiny bit of 
convenience over an iterator, so an unmodifiable Queue provides hardly any value 
over Collection. Thus the rationale in JDK-5030930 for not providing an 
unmodifiable Queue makes sense.


Deque is considerably richer than Queue, not only in mutator methods. It also 
adds access methods for both ends (null-returning and throwing), with better 
names, plus a descending iterator. That might make it worthwhile reconsidering 
an unmodifiable Deque.


s'marks

Re: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

2020-04-29 Thread Stuart Marks


Hi Dmytro,

Callers of an API performing explicit synchronization, along with the 
synchronized collections wrappers, have mostly fallen into disuse since the 
introduction of the java.util.concurrent collections.


Multiple threads can either interact directly on a concurrent collection, or the 
developer can provide an intermediate object (not a collection) that does 
internal locking, and that exports the right set of thread-safe APIs to callers. 
I'm thus skeptical of the utility of enhancing these wrapper classes with 
additional APIs.


Do you have a use case that's difficult to handle any other way?

s'marks



On 4/29/20 12:58 AM, dmytro sheyko wrote:

Hello,

Have you ever discussed to make field mutex in synchronized collections
accessible?

Javadoc for Collections#synchronizedSortedSet suggest to iterate collection
this way:

   SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
   SortedSet s2 = s.headSet(foo);
   ...
   synchronized (s) {  // Note: s, not s2!!!
   Iterator i = s2.iterator(); // Must be in the synchronized block
   while (i.hasNext())
   foo(i.next());
   }

I.e. in order to iterate subset, we also need a reference to the whole set,
which is not really convenient. How about to make it possible to write:

   SortedSet s2 = s.headSet(foo);
   ...
   synchronized (Collections.getSyncRoot(s2)) {  // Note:
Collections.getSyncRoot(s2)
   Iterator i = s2.iterator(); // Must be in the synchronized block
   while (i.hasNext())
   foo(i.next());
   }

Also I think it would be convenient to let to provide custom sync root when
synchronized collection is created.
E.g.

   Object customSyncRoot = new Object();
   SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
customSyncRoot);

What do you think about this?

Regards,
Dmytro

Re: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException

2020-04-29 Thread Stuart Marks

The change looks fine. Although this is in a normative portion of the 
specification, the nature of the change is purely editorial, so I don't think it 
needs a CSR.


Do you need a sponsor?

s'marks

On 4/29/20 2:57 AM, Jayashree Sk1 wrote:

Hi All,
 Please find the below changes for the issues 
https://bugs.openjdk.java.net/browse/JDK-6415694.
It is a description change, which was already approved by the reviewer.

Thanks!

diff -r 59b5bd9a7168 
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 
16 02:16:49 2020 -0400
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 
30 15:45:43 2020 +0530
@@ -26,8 +26,8 @@
  
  /**

   * An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
   *
   * @since   1.1
   * @author  Ann Wollrath

Re: RFR: 7147994 Hashtable rehash‎(‎) javadoc describes implementation details

2020-04-29 Thread Stuart Marks

The bug report states that this method specification describes implementation 
details, with the implication that implementation details should be avoided and 
that abstract specifications (contracts or invariants) should be provided 
instead. The alternative wording from the bug report removes the implementation 
details and replaces them with some informative text.


However, looking more closely about this change, I think it's wrong.

This is a protected method, so it can be overridden or called by a subclass. As 
such, the method specification should provide information necessary for 
subclasses to be implemented correctly, in particular, about "self-use" of the 
overridable methods from other parts of the superclass implementation. (See 
Bloch, Effective Java 3/e, Item 19. See also AbstractCollection.removeAll() for 
an example of this in practice.)


I think the bug report is wrong because it suggests removing implementation 
details, when in fact this is a place where implementation details ought to be 
provided.


(One might argue that more implementation details should be provided here, but 
it's not clear that Hashtable was actually designed for subclassing. That said, 
it can be subclassed, and there are surely many subclasses out there relying on 
all kinds of behavior of Hashtable. It's probably not worth trying to document 
all of it though.)


So, I'm inclined to close this issue as Won't Fix.

s'marks


On 4/29/20 3:02 AM, Jayashree Sk1 wrote:

Hi All,
 Please find the below changes for the issues 
https://bugs.openjdk.java.net/browse/JDK-7147994
It is a description change, which was already approved by the reviewer.

Thanks!

diff -r 59b5bd9a7168 src/java.base/share/classes/java/util/Hashtable.java
--- a/src/java.base/share/classes/java/util/Hashtable.java  Mon Mar 16 
02:16:49 2020 -0400
+++ b/src/java.base/share/classes/java/util/Hashtable.java  Mon Mar 30 
15:45:43 2020 +0530
@@ -399,9 +399,9 @@
  /**
   * Increases the capacity of and internally reorganizes this
   * hashtable, in order to accommodate and access its entries more
- * efficiently.  This method is called automatically when the
- * number of keys in the hashtable exceeds this hashtable's capacity
- * and load factor.
+ * efficiently.  This method is called to increase the capacity of and
+ * internally reorganize this hashtable in order to more efficiently
+ * accommodate and access its entries.
   */
  @SuppressWarnings("unchecked")
  protected void rehash() {

Re: RFR: 7147994 Hashtable rehash‎(‎) javadoc describes implementation details

2020-04-30 Thread Stuart Marks


OK, great, I've closed out the bug.

s'marks

On 4/29/20 10:29 PM, Jayashree Sk1 wrote:


Stuart, Thanks for all the details.
All you have said makes sense to me, I have no contention for closing this 
issue as Won't Fix (am not related to originator, picked this issue up as 
starters to understand the contribution process)


Regards!
Jay



-----Stuart Marks  wrote: -
To: Jayashree Sk1 
From: Stuart Marks 
Date: 04/30/2020 09:53AM
Cc: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 7147994 Hashtable rehash‎(‎) javadoc describes 
implementation details

The bug report states that this method specification describes implementation
details, with the implication that implementation details should be avoided and
that abstract specifications (contracts or invariants) should be provided
instead. The alternative wording from the bug report removes the implementation
details and replaces them with some informative text.

However, looking more closely about this change, I think it's wrong.

This is a protected method, so it can be overridden or called by a subclass. As
such, the method specification should provide information necessary for
subclasses to be implemented correctly, in particular, about "self-use" of the
overridable methods from other parts of the superclass implementation. (See
Bloch, Effective Java 3/e, Item 19. See also AbstractCollection.removeAll() for
an example of this in practice.)

I think the bug report is wrong because it suggests removing implementation
details, when in fact this is a place where implementation details ought to be
provided.

(One might argue that more implementation details should be provided here, but
it's not clear that Hashtable was actually designed for subclassing. That said,
it can be subclassed, and there are surely many subclasses out there relying on
all kinds of behavior of Hashtable. It's probably not worth trying to document
all of it though.)

So, I'm inclined to close this issue as Won't Fix.

s'marks


On 4/29/20 3:02 AM, Jayashree Sk1 wrote:

Hi All,
  Please find the below changes for the issues 
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D7147994&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k&m=bNY044TQSELJWMafSRxC-62CyHMVnMxboXMG0qqU6CU&s=V9rN7MCO4rArlV8tRjEn44_Kl_tCH2h-xjqakCvpBxY&e=
It is a description change, which was already approved by the reviewer.

Thanks!

diff -r 59b5bd9a7168 src/java.base/share/classes/java/util/Hashtable.java
--- a/src/java.base/share/classes/java/util/Hashtable.java  Mon Mar 16 
02:16:49 2020 -0400
+++ b/src/java.base/share/classes/java/util/Hashtable.java  Mon Mar 30 
15:45:43 2020 +0530
@@ -399,9 +399,9 @@
   /**
* Increases the capacity of and internally reorganizes this
* hashtable, in order to accommodate and access its entries more
- * efficiently.  This method is called automatically when the
- * number of keys in the hashtable exceeds this hashtable's capacity
- * and load factor.
+ * efficiently.  This method is called to increase the capacity of and
+ * internally reorganize this hashtable in order to more efficiently
+ * accommodate and access its entries.
*/
   @SuppressWarnings("unchecked")
   protected void rehash() {

Re: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

2020-04-30 Thread Stuart Marks

The general rule in the collections that wrappers and views don't divulge their 
backing collections. The reason is fairly obvious for things like the checked 
wrappers and unmodifiable wrappers, but it's equally important for various view 
collections like those of Lists, Sets, and Maps. If there were something like 
getSyncRoot() it could be used to break encapsulation. For example:

Map map = Collections.synchronizedMap(...);
someMethod(map.values());

Currently, the caller is assured that someMethod() can only see the values of 
the map. Also, as you observe, someMethod() can't safely iterate that collection 
using an iterator or stream. If getSyncRoot() were introduced to address that issue,

void someMethod(Collection values) {
@SuppressWarnings("unchecked")
var map = (Map) Collections.getSyncRoot(values);
// I now have access to map's keys and can put new entries, mwahaha!
}

Undoubtedly there are ways we can avoid this, but the cost is designing yet more 
complicated APIs in an area where it provides little value. I think it's pretty 
unlikely that we'll do anything like this, or variants that allow the caller to 
provide an external lock at creation time, such as proposed in JDK-4335520. 
There's a pretty fundamental clash between external locking and encapsulation, 
and the platform has shifted to pursuing other approaches for concurrency.

Legacy code that needs to iterate collection views might find some luck 
replacing iterator loops with bulk methods like forEach() or removeIf().

s'marks

On 4/30/20 1:34 AM, dmytro sheyko wrote:

Hi Stuart,

In general I agree with you regarding synchronized collections. But nevertheless 
there is a lot of legacy code that still uses synchronized wrappers. And 
sometimes synchronization is done on wrong lock object, which leads to 
relatively rare but extremely hard to reproduce and troubleshoot errors. 
Reworking whole this legacy stuff is risky. But if we had means to get the right 
lock object for given synchronized collections, this would help us to make the 
fixes more localized and hence safe. That's why I would like to know the reason, 
why this has not been done earlier, and is there hope/plan this will be done in 
near future.

Thank you,
Dmytro

On Thu, Apr 30, 2020 at 6:36 AM Stuart Marks <mailto:stuart.ma...@oracle.com>> wrote:

Hi Dmytro,

Callers of an API performing explicit synchronization, along with the
synchronized collections wrappers, have mostly fallen into disuse since the
introduction of the java.util.concurrent collections.

Multiple threads can either interact directly on a concurrent collection, or
the
developer can provide an intermediate object (not a collection) that does
internal locking, and that exports the right set of thread-safe APIs to
callers.
I'm thus skeptical of the utility of enhancing these wrapper classes with
additional APIs.

Do you have a use case that's difficult to handle any other way?

s'marks

On 4/29/20 12:58 AM, dmytro sheyko wrote:
 > Hello,
 >
 > Have you ever discussed to make field mutex in synchronized collections
 > accessible?
 >
 > Javadoc for Collections#synchronizedSortedSet suggest to iterate 
collection
 > this way:
 >
 >    SortedSet s = Collections.synchronizedSortedSet(new TreeSet());
 >    SortedSet s2 = s.headSet(foo);
 >        ...
 >    synchronized (s) {  // Note: s, not s2!!!
 >        Iterator i = s2.iterator(); // Must be in the synchronized block
 >        while (i.hasNext())
 >            foo(i.next());
 >    }
 >
 > I.e. in order to iterate subset, we also need a reference to the whole 
set,
 > which is not really convenient. How about to make it possible to write:
 >
 >    SortedSet s2 = s.headSet(foo);
 >        ...
 >    synchronized (Collections.getSyncRoot(s2)) {  // Note:
 > Collections.getSyncRoot(s2)
 >        Iterator i = s2.iterator(); // Must be in the synchronized block
 >        while (i.hasNext())
 >            foo(i.next());
 >    }
 >
 > Also I think it would be convenient to let to provide custom sync root 
when
 > synchronized collection is created.
 > E.g.
 >
 >    Object customSyncRoot = new Object();
 >    SortedSet s = Collections.synchronizedSortedSet(new TreeSet(),
 > customSyncRoot);
 >
 > What do you think about this?
 >
 > Regards,
 > Dmytro
 >

Re: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException

2020-04-30 Thread Stuart Marks


Hi Jay,

OK, welcome! I'll sponsor this changeset for you. It's pretty simple, so I'll 
just paste the exported changeset below. (For complicated changesets, you'll 
have to ask a sponsor to host a webrev on cr.openjdk.java.net for you.) Please 
check the Contributed-by line in the changeset to make sure the attribution is 
correct. I just copied the From: line from your email, but if you want it to be 
different, please let me know.


I'll update the bug report with a pointer to this email thread.

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

Could I get a Reviewer to review this? Thanks.

(I noticed that the anachronistic  markup is used here instead of the 
preferred {@code ...} javadoc tag. However,  is used all over this 
package, so I didn't think that changing it in just one place would be helpful.)


I'll also note again that since this is merely an editorial wording change to 
the specification, I don't think it needs a CSR request.


Thanks,

s'marks





# HG changeset patch
# User smarks
# Date 1588290431 25200
#  Thu Apr 30 16:47:11 2020 -0700
# Node ID 1f5b3876d1500a2e77d6e39d079649964ba6dd26
# Parent  fe6548dc92de4c875467c968ee37e7c9bbee1697
6415694: Clarification in Javadoc for java.rmi.AlreadyBoundException
Reviewed-by: XXX
Contributed-by: Jayashree Sk1 

diff -r fe6548dc92de -r 1f5b3876d150 
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java	Thu Apr 30 
15:21:15 2020 -0700
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java	Thu Apr 30 
16:47:11 2020 -0700

@@ -26,8 +26,8 @@

 /**
  * An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
  *
  * @since   1.1
  * @author  Ann Wollrath




On 4/29/20 10:24 PM, Jayashree Sk1 wrote:

Thanks for the review comment Stuart. Yes, I will need a sponsor, this is my 
first time here in OpenJDK.


Regards!
Jay
  
-Stuart Marks  wrote: -

To: Jayashree Sk1 
From: Stuart Marks 
Date: 04/30/2020 09:22AM
Cc: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 6415694 Clarification in Javadoc for 
java.rmi.AlreadyBoundException

The change looks fine. Although this is in a normative portion of the
specification, the nature of the change is purely editorial, so I don't think it
needs a CSR.

Do you need a sponsor?

s'marks

On 4/29/20 2:57 AM, Jayashree Sk1 wrote:

Hi All,
  Please find the below changes for the issues 
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D6415694&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k&m=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk&s=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4&e=
 .
It is a description change, which was already approved by the reviewer.

Thanks!

diff -r 59b5bd9a7168 
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 
16 02:16:49 2020 -0400
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 
30 15:45:43 2020 +0530
@@ -26,8 +26,8 @@
   
   /**

* An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
*
* @since   1.1
* @author  Ann Wollrath

RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-01 Thread Stuart Marks


Hi all,

I'm taking another swing at this one. I've taken a minimal approach compared to 
my previous proposals.


Briefly, AbstractSet.removeAll switches from iterating this collection and 
calling contains() on its argument, to iterating the argument and calling 
this.contains(), if the argument collection is smaller than this collection. 
This attempt at optimization is incorrect, because this collection and the 
argument collection might have different semantics for contains().


There is a confounding performance problem, which is that if the argument is a 
List, contains() is generally linear, which can result in pathologically slow 
(O(m*n)) performance. Because of the iteration-switching above, this performance 
problem can appear and disappear based on the relative sizes, which is startling.


The fix for the semantic problem is simply to remove the attempted optimization 
from AbstractSet. This comports with the specification of Collection.removeAll 
and Set.removeAll, which imply that contains() is called on the argument 
collection. This allows sets to inherit the implementation in 
AbstractCollection.removeAll. In addition, this ensures that removeAll is now 
always the complement of retainAll (as it should be), which it sometimes was not 
when the optimization was in place.


IdentityHashMap's keySet and entrySet views were broken by this optimization, so 
they had to override removeAll with copies of the implementation from 
AbstractCollection. Since they can now inherit from AbstractCollection, these 
overrides have been removed.


This leaves a potential performance problem with removeAll when the argument is 
a List. To mitigate this, HashSet.removeAll switches to iterating the argument 
if the argument is a List. This is safe, as both HashSet and List use the same 
semantics for contains() (at least, as far as I know).


I've opted not to pursue size-based optimizations at this time, since they 
provide only incremental benefit, whereas the pathological performance problem 
mentioned above is the primary issue. Also, it's actually quite difficult to 
determine when it's safe to switch iteration.


Finally, I've added performance notes to the specifications of containsAll(), 
removeAll(), and retainAll(), warning about potential performance problems that 
can occur with repeated calls to contains() made by these methods.


Bug:

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

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/6394757/webrev.2/

Previous discussions:

http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-July/007125.html


http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/058140.html


http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/058378.html

http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060007.html

http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060147.html

Thanks,

s'marks

Re: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException

2020-05-01 Thread Stuart Marks


Thanks Roger.

I figure I could have gotten away without an additional reviewer. :-) However, I 
asked for a review because I wanted someone to cross-check my decision not to 
file a CSR for this, despite most specification changes requiring a CSR. I also 
wanted to model behavior for new community members.


s'marks

On 5/1/20 7:57 AM, Roger Riggs wrote:

+1,

BTW, Stuart you are a Reviewer, no need for a 2nd.

Roger


On 5/1/20 12:02 AM, Stuart Marks wrote:

Hi Jay,

OK, welcome! I'll sponsor this changeset for you. It's pretty simple, so I'll 
just paste the exported changeset below. (For complicated changesets, you'll 
have to ask a sponsor to host a webrev on cr.openjdk.java.net for you.) Please 
check the Contributed-by line in the changeset to make sure the attribution is 
correct. I just copied the From: line from your email, but if you want it to 
be different, please let me know.


I'll update the bug report with a pointer to this email thread.

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

Could I get a Reviewer to review this? Thanks.

(I noticed that the anachronistic  markup is used here instead of the 
preferred {@code ...} javadoc tag. However,  is used all over this 
package, so I didn't think that changing it in just one place would be helpful.)


I'll also note again that since this is merely an editorial wording change to 
the specification, I don't think it needs a CSR request.


Thanks,

s'marks





# HG changeset patch
# User smarks
# Date 1588290431 25200
#  Thu Apr 30 16:47:11 2020 -0700
# Node ID 1f5b3876d1500a2e77d6e39d079649964ba6dd26
# Parent  fe6548dc92de4c875467c968ee37e7c9bbee1697
6415694: Clarification in Javadoc for java.rmi.AlreadyBoundException
Reviewed-by: XXX
Contributed-by: Jayashree Sk1 

diff -r fe6548dc92de -r 1f5b3876d150 
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java Thu Apr 
30 15:21:15 2020 -0700
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java Thu Apr 
30 16:47:11 2020 -0700

@@ -26,8 +26,8 @@

 /**
  * An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
  *
  * @since   1.1
  * @author  Ann Wollrath




On 4/29/20 10:24 PM, Jayashree Sk1 wrote:
Thanks for the review comment Stuart. Yes, I will need a sponsor, this is my 
first time here in OpenJDK.



Regards!
Jay
  -Stuart Marks  wrote: -
To: Jayashree Sk1 
From: Stuart Marks 
Date: 04/30/2020 09:22AM
Cc: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 6415694 Clarification in Javadoc for 
java.rmi.AlreadyBoundException


The change looks fine. Although this is in a normative portion of the
specification, the nature of the change is purely editorial, so I don't think it
needs a CSR.

Do you need a sponsor?

s'marks

On 4/29/20 2:57 AM, Jayashree Sk1 wrote:

Hi All,
  Please find the below changes for the issues 
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D6415694&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k&m=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk&s=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4&e= 
.

It is a description change, which was already approved by the reviewer.

Thanks!

diff -r 59b5bd9a7168 
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java Mon Mar 
16 02:16:49 2020 -0400
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java Mon Mar 
30 15:45:43 2020 +0530

@@ -26,8 +26,8 @@
      /**
    * An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
    *
    * @since   1.1
    * @author  Ann Wollrath

Re: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

2020-05-01 Thread Stuart Marks


On 5/1/20 2:02 AM, dmytro sheyko wrote:
The checked, synchronized and unmodifiable wrappers in some cases store backing 
collection in more than one fields.


Thus `UnmodifiableList` has
1. its own field `List list` and
2. `Collection c`, which it inherits from 
`UnmodifiableCollection`.

Also `UnmodifiableNavigableSet` has even 3 duplicated fields:
1. its own `NavigableSet ns`,
2. `SortedSet ss` from `UnmodifiableSortedSet` and
3. `Collection c` from `UnmodifiableCollection`.

Isn't it worth to get rid of such duplication? (at least for unmodifiable 
collections). This may affect serialization, but I believe it's still possible 
preserve serialization backward compatible, if it's necessary.

Or is it done intentionally?


Interesting. I went through some of the history here, in particular the 
core-libs-dev review threads of JDK-7129185 [1] which was the last time 
significant work was done on the wrappers. There was no mention of the duplicate 
references in any of the reviews. I suspect the wrappers introduced in this 
changeset (e.g., UnmodifiableNavigableSet) copied the style from existing 
wrappers, which also used this style.


I was able to look through the old (non open source) history of the JDK, and I 
found that this style of having a redundant field in a wrapper subtype was 
introduced in JDK 1.2 in 1998 along with the original collections implementation.


I suspect it was done this way for convenience. Certainly the wrapper subclasses 
have access to the field from the superclass. But to use it they there would 
have to be a cast at every call site that wanted to use a subclass method. This 
would certainly make the wrapper code more verbose, and it might even slow 
things down a bit with checkcast bytecodes and such.


While trying to save space is laudable, compatibility with existing serial forms 
needs to be preserved. Doing this would require adding serialPersistentFields 
arrays and readObject() and writeObject() methods to every one of the wrapper 
classes. This is fairly tedious and error-prone. I'm not sure it's worth it.


s'marks


[1] https://bugs.openjdk.java.net/browse/JDK-7129185

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-04 Thread Stuart Marks

quality semantics of the receiver:
containsAll, equals, remove, contains. To let TreeSet.removeAll be
inconsistent with all these methods seems worse than it being inconsistent
with the less commonly used TreeSet.retainAll.

The conclusion is that I think it would be better if TreeSet or
AbstractSet gets an implementation of removeAll which iterates over the
argument collection.

Best regards,
Jens Lideström

[1]:
https://mail.openjdk.java.net/pipermail/core-libs-dev/2019-July/061581.html

On 2020-05-01 22:01, Stuart Marks wrote:

Hi all,

I'm taking another swing at this one. I've taken a minimal approach
compared to my previous proposals.

Briefly, AbstractSet.removeAll switches from iterating this collection and
calling contains() on its argument, to iterating the argument and calling
this.contains(), if the argument collection is smaller than this
collection. This attempt at optimization is incorrect, because this
collection and the argument collection might have different semantics for
contains().

There is a confounding performance problem, which is that if the argument
is a List, contains() is generally linear, which can result in
pathologically slow (O(m*n)) performance. Because of the
iteration-switching above, this performance problem can appear and
disappear based on the relative sizes, which is startling.

The fix for the semantic problem is simply to remove the attempted
optimization from AbstractSet. This comports with the specification of
Collection.removeAll and Set.removeAll, which imply that contains() is
called on the argument collection. This allows sets to inherit the
implementation in AbstractCollection.removeAll. In addition, this ensures
that removeAll is now always the complement of retainAll (as it should
be), which it sometimes was not when the optimization was in place.

IdentityHashMap's keySet and entrySet views were broken by this
optimization, so they had to override removeAll with copies of the
implementation from AbstractCollection. Since they can now inherit from
AbstractCollection, these overrides have been removed.

This leaves a potential performance problem with removeAll when the
argument is a List. To mitigate this, HashSet.removeAll switches to
iterating the argument if the argument is a List. This is safe, as both
HashSet and List use the same semantics for contains() (at least, as far
as I know).

I've opted not to pursue size-based optimizations at this time, since they
provide only incremental benefit, whereas the pathological performance
problem mentioned above is the primary issue. Also, it's actually quite
difficult to determine when it's safe to switch iteration.

Finally, I've added performance notes to the specifications of
containsAll(), removeAll(), and retainAll(), warning about potential
performance problems that can occur with repeated calls to contains() made
by these methods.

Bug:

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

Webrev:

     http://cr.openjdk.java.net/~smarks/reviews/6394757/webrev.2/

Previous discussions:


http://mail.openjdk.java.net/pipermail/core-libs-dev/2011-July/007125.html



http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-January/058140.html



http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-February/058378.html



     http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060007.html

     http://mail.openjdk.java.net/pipermail/core-libs-dev/2019-May/060147.html

Thanks,

s'marks

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-04 Thread Stuart Marks

On 5/1/20 10:41 PM, Jason Mehrens wrote:

1. I assume you are using "c instanceof List" instead of "!(c instanceof Set)"
to correctly handle IdentitityHashMap.values()? The instanceof List seems like safe choice but it
is too bad we can still fool that check by wrapping List as an unmodifiableCollection. If
splitIterator().characteristics() could tell us that the collection used identity comparisons I
think we would be able to determine if it was safe to swap the ordering in the general case as we
could check for IDENTITY, SORTED, and comparator equality.

I'm using "instance List", not for the reason of IdentityHashMap.values()
specifically (though that's a good example), but mainly to try to be minimal.
While I think getting the semantics right takes priority, the change does impact
performance, so I want to reintroduce the performance heuristic in the safest
manner possible. Checking for !Set seems dangerous, as there might be any number
of Collections out there that aren't Sets and that aren't consistent with
equals. Checking for instanceof List seemed like the safest and most minimal
thing to do.

In fact, I'm not even sure how safe List is! It's certainly possible for someone
to have a List that isn't consistent with equals. Such a thing might violate the
List contract, but that hasn't stopped people from implementing such things
(outside the JDK).

I also toyed around with various additional tests for when it would be
profitable to switch iteration to the smaller collection. This could be applied
to a variety of consistent-with-equals set implementations in the JDK. The
benefits of swapping the iteration is more modest in these cases compared to
List, however. Avoiding repeated List.contains() helps avoid quasi-quadratic
(O(M*N)) performance. Swapping iteration order of sets gets us only the smaller
of O(M) vs O(N), which is still linear.

Also, as you noted, this heuristic is easily defeated by things like the
collection wrappers.

2. Should code applied to HashSet.removeAll also be applied to
HashMap.keySet().removeAll and HashMap.entrySet().removeAll?
Collections::newSetFromMap will end up having different behavior if it is not
consistently applied.

I think the *results* will be consistent, but the *performance* might not be
consistent.

But these cases could result in pathological performance if removeAll(list) is
called, so I'm a bit concerned about them. If we agree that "instanceof List" is
a reasonable heuristic, then I don't have any objection in principle to adding
it to these locations as well. But I want to be careful about sprinkling ad hoc
policies like this around the JDK.

I note that the erroneous size-based optimization was copied into, and therefore
needs to be removed from ConcurrentSkipListSet (JDK-8218945) and the set views
of CHM (JDK-8219069). I'd more concerned about getting these cleaned up in the
short term.

3. Not to derail this thread but do think we need a new JIRA ticket to address
Collections.disjoint? Seems like it has similar sins of calling size and using
"!(c2 instanceof Set)" but I don't want to muddy the waters by covering any
solutions to that method in this thread.

Yeah I'm not sure what to do about Collections.disjoint().

Note that there are some statements in bug reports (possibly even made by me!)
that assert that Collections.disjoint should be consistent with Set.removeAll. I
don't think that's the case. As discussed elsewhere, removeAll() needs to be
consistent about relying on the membership semantics of the argument collection.

As Collections.disjoint currently stands, it has the big "Care must be
exercised" disclaimer in its spec, and it goes to some length to make a bunch of
tests and vary the iteration accordingly. The programmer can get a speedup using
disjoint() compared to copying a set and then calling retainAll(), provided
sufficient care is taken. Maybe that's an acceptable tradeoff.

If you have any ideas about how disjoint()'s spec or implementation could be
improved, you could file a JIRA issue, or I could file one if you sent me the info.

s'marks

Re: [15] RFR: 8244459: Optimize the hash map size in LocaleProviderAdapters

2020-05-05 Thread Stuart Marks


Hm, interesting, good catch Peter! Very subtle. The time-honored

(int) (expected / 0.75f) + 1

appears in several places around the JDK. I think most people (including me) 
just copy it, because it's "good enough" for most cases.


I'm a little concerned about

(expectedSize * 4 + 2) / 3

because that wraps around to negative starting at about 2^29. Might I suggest 
the following instead?


(int) Math.ceil(expectedSize / 0.75)

This expresses the actual intent more clearly, I think, and its results match 
Peter's expression for the range less than 2^29. It also saturates at 
Integer.MAX_VALUE instead of going negative. It does use double precision, 
though, as there's no float version of ceil(). At this point I think this is a 
small disadvantage.


**

As for Naoto's changeset, I don't think it should be held up while we bikeshed 
this. :-) I'm ok with whatever he decides.


s'marks




On 5/5/20 1:26 PM, Peter Levart wrote:

There's more...


Guava (and JDK in copy constructor) formula:

     (int) ((float) expectedSize / 0.75f + 1.0f)


is not the same as Naoto's formula:

     (int) (expectedSize / 0.75f) + 1


Notice that Naoto does addition of 1 in integer arithmetic after conversion to 
int, while Guava/JDK does in floating point before conversion to int. If I put 
Naoto's formula into my test program, no undercalculations are detected.



So while Naoto's formula is sub-optimal for expectedSizes that are multiples of 
3, the Guava/JDK also has a bug.



Regards, Peter


On 5/5/20 10:01 PM, Peter Levart wrote:



On 5/5/20 9:41 PM, Martin Buchholz wrote:

Hi Peter,

Are you saying guava has a tiny bug?



If it was just 1 too much when expected size is a multiple of 3 then that 
would not be a bug, just sub-optimal calculation. And the same calculation is 
performed also in JDK when a copy constructor is called for example.



But I investigated further and what I found could be considered a bug. 
Sometimes, the following expression:



(int) ((float) expectedSize / 0.75f + 1.0f)


...calculates a value that is not enough (due to floating point arithmetic and 
conversion to int) to store the expectedSize elements into the HashMap without 
re-hashing.



What HashMap does with initialCapacity parameter is to round it up to nearest 
power of 2:


    static int tableSizeFor(int cap) {
    int n = -1 >>> Integer.numberOfLeadingZeros(cap - 1);
    return (n < 0) ? 1 : (n >= MAXIMUM_CAPACITY) ? MAXIMUM_CAPACITY : n + 1;
    }

then it uses this as the initial backing table size. From that table size it 
calculates the threshold value:


    static int threshold(int cap) {
    float ft = (float) cap * 0.75f;
    return (cap < MAXIMUM_CAPACITY && ft < (float) MAXIMUM_CAPACITY ?
    (int) ft : Integer.MAX_VALUE);
    }

... and uses it as the max. number of elements that a HashMap can hold before 
it is re-hashed. So I did the following test (comparing the effectiveness of 
above formula with alternative (expectedSize*4+2)/3 formula):



public class HMTest {
    static final int MAXIMUM_CAPACITY = 1 << 30;

    static int tableSizeFor(int cap) {
    int n = -1 >>> Integer.numberOfLeadingZeros(cap - 1);
    return (n < 0) ? 1 : (n >= MAXIMUM_CAPACITY) ? MAXIMUM_CAPACITY : n + 1;
    }

    static int threshold(int cap) {
    float ft = (float) cap * 0.75f;
    return (cap < MAXIMUM_CAPACITY && ft < (float) MAXIMUM_CAPACITY ?
    (int) ft : Integer.MAX_VALUE);
    }

    public static void main(String[] args) {
    for (int expectedSize = 0; expectedSize < (Integer.MAX_VALUE - 2) / 4; 
expectedSize++) {

    int cap1 = (int) ((float) expectedSize / 0.75f + 1.0f);
    int cap2 = (expectedSize * 4 + 2) / 3;
    int ts1 = tableSizeFor(cap1);
    int ts2 = tableSizeFor(cap2);
    int th1 = threshold(ts1);
    int th2 = threshold(ts2);

    if (th1 < expectedSize || th2 < expectedSize) {
    System.out.printf("%d: (%d, %d, %d)%s (%d, %d, %d)%s\n",
    expectedSize,
    cap1, ts1, th1, (th1 < expectedSize) ? "!" : " ",
    cap2, ts2, th2, (th2 < expectedSize) ? "!" : " "
    );
    }
    }
    }
}


And what this prints is the following:


25165825: (33554432, 33554432, 25165824)! (33554434, 67108864, 50331648)
50331649: (67108864, 67108864, 50331648)! (67108866, 134217728, 100663296)
50331650: (67108864, 67108864, 50331648)! (67108867, 134217728, 100663296)
100663297: (134217728, 134217728, 100663296)! (134217730, 268435456, 201326592)
100663298: (134217728, 134217728, 100663296)! (134217731, 268435456, 201326592)
100663299: (134217728, 134217728, 100663296)! (134217732, 268435456, 201326592)
100663300: (134217728, 134217728, 100663296)! (134217734, 268435456, 201326592)
201326593: (268435456, 268435456, 201326592)! (268435458, 536870912, 402653184)
201326594: (268435456, 268435456, 20132659

Re: RFR: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException

2020-05-06 Thread Stuart Marks


OK, pushed:

http://hg.openjdk.java.net/jdk/jdk/rev/6b4e4ffe01ab

Thanks for your contribution!

s'marks

On 5/4/20 1:50 AM, Jayashree Sk1 wrote:


Hi Stuart,

Yup Contributed-by as picked from my email : jayashreesk .
Will keep in mind about the complicated change-sets.


Thanks
Jay
  
-Stuart Marks  wrote: -

To: Jayashree Sk1 
From: Stuart Marks 
Date: 05/01/2020 09:32AM
Cc: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 6415694 Clarification in Javadoc for 
java.rmi.AlreadyBoundException

Hi Jay,

OK, welcome! I'll sponsor this changeset for you. It's pretty simple, so I'll
just paste the exported changeset below. (For complicated changesets, you'll
have to ask a sponsor to host a webrev on cr.openjdk.java.net for you.) Please
check the Contributed-by line in the changeset to make sure the attribution is
correct. I just copied the From: line from your email, but if you want it to be
different, please let me know.

I'll update the bug report with a pointer to this email thread.


https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D6415694&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k&m=9YMcjPnAAEf40XGqmcEU_Uiyg_xCVlOU26-U56hEb4A&s=ueOfMpgrTsTZc7xg22AdzVskohCya_stq_WLTREzw24&e=

Could I get a Reviewer to review this? Thanks.

(I noticed that the anachronistic  markup is used here instead of the
preferred {@code ...} javadoc tag. However,  is used all over this
package, so I didn't think that changing it in just one place would be helpful.)

I'll also note again that since this is merely an editorial wording change to
the specification, I don't think it needs a CSR request.

Thanks,

s'marks





# HG changeset patch
# User smarks
# Date 1588290431 25200
#  Thu Apr 30 16:47:11 2020 -0700
# Node ID 1f5b3876d1500a2e77d6e39d079649964ba6dd26
# Parent  fe6548dc92de4c875467c968ee37e7c9bbee1697
6415694: Clarification in Javadoc for java.rmi.AlreadyBoundException
Reviewed-by: XXX
Contributed-by: Jayashree Sk1 

diff -r fe6548dc92de -r 1f5b3876d150
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaThu Apr 
30
15:21:15 2020 -0700
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaThu Apr 
30
16:47:11 2020 -0700
@@ -26,8 +26,8 @@

   /**
* An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
*
* @since   1.1
* @author  Ann Wollrath




On 4/29/20 10:24 PM, Jayashree Sk1 wrote:

Thanks for the review comment Stuart. Yes, I will need a sponsor, this is my 
first time here in OpenJDK.


Regards!
Jay
   
-Stuart Marks  wrote: -

To: Jayashree Sk1 
From: Stuart Marks 
Date: 04/30/2020 09:22AM
Cc: core-libs-dev@openjdk.java.net
Subject: [EXTERNAL] Re: RFR: 6415694 Clarification in Javadoc for 
java.rmi.AlreadyBoundException

The change looks fine. Although this is in a normative portion of the
specification, the nature of the change is purely editorial, so I don't think it
needs a CSR.

Do you need a sponsor?

s'marks

On 4/29/20 2:57 AM, Jayashree Sk1 wrote:

Hi All,
   Please find the below changes for the issues 
https://urldefense.proofpoint.com/v2/url?u=https-3A__bugs.openjdk.java.net_browse_JDK-2D6415694&d=DwICaQ&c=jf_iaSHvJObTbx-siA1ZOg&r=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k&m=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk&s=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4&e=
 .
It is a description change, which was already approved by the reviewer.

Thanks!

diff -r 59b5bd9a7168 
src/java.rmi/share/classes/java/rmi/AlreadyBoundException.java
--- a/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 
16 02:16:49 2020 -0400
+++ b/src/java.rmi/share/classes/java/rmi/AlreadyBoundException.javaMon Mar 
30 15:45:43 2020 +0530
@@ -26,8 +26,8 @@

/**

 * An AlreadyBoundException is thrown if an attempt
- * is made to bind an object in the registry to a name that already
- * has an associated binding.
+ * is made to bind an object to a name that already
+ * has an associated binding in the registry.
 *
 * @since   1.1
 * @author  Ann Wollrath

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-06 Thread Stuart Marks

On 5/4/20 6:44 PM, Alan Snyder wrote:

What problem are you trying to solve?

The Collections framework explicitly does not support custom membership
semantics. If you think it should, file an RFE and create a JEP. It’s not a
small change and tinkering is not the way to get there.

There are already three different kinds of sets in the JDK that support
different membership semantics: sets derived from IdentityHashMap, ordinary sets
like HashSet, and the SortedSet/NavigableSet family. Arguably the last already
supports custom membership semantics, as it's possible for callers to provide
their own comparators. I'm trying to fix semantic bugs in the way various
collection operations handle situations with mixed membership semantics, and
secondarily, to avoid pathological performance problems that have arisen.

If your present concern is performance surprises, be aware that your new
proposal has the same problem as the old one. Although it removes some
data-dependent performance surprises, it adds a potential JDK-dependent
performance surprise. The data-dependent performance issues can be detected by
testing, but no amount of testing can alert a developer to the possibility that
an unexpected implementation change in a future JDK might cause a big
performance hit.

You may have mis-remembered the performance problem that I am concerned about.
It involves using removeAll to remove a relatively small number of elements from
a large, hash based collection. The performance hit is the need to iterate over
the entire collection and test every element regardless of the number of
elements being removed. Although the performance problem may be exacerbated when
the argument is a List, the problem exists for any collection that is much
smaller than the target collection.

You're conflating two different parts of the performance issue.

This is illustrated in an article that Jon Skeet posted back in 2010, [1] which
is linked from JDK-6982173. Briefly, Skeet observed that a removeAll using a
List of 300,000 elements could take nearly 3 minutes, whereas iterating a
HashSet of 1,000,000 elements would take only 38ms.

(These numbers are from 2010, and hardware is certainly different today, and
these aren't rigorous benchmarks. However, an informal benchmark that shows the
difference between 3 minutes and 38ms is a pretty clear demonstration of a
performance problem.)

Taking 3 minutes for this kind of operation is clearly pathological behavior,
which is what I'm trying to address. Although it seems impossible to prevent it
from ever happening, putting some special cases for handling List into places
such as HashSet.removeAll would seem to cover the most of the common cases.

It's true that if you're removing a small set from a large set, iterating the
"wrong" set might take 38ms instead of a much smaller time (probably
microseconds). This would indeed be a performance regression. (It might also be
an improvement in correctness, if the sets have different membership contracts.)

The fact is that there are performance regressions from one JDK release to the
next. Sometimes they're introduced by accident, and we try to address these
where possible. Sometimes they're introduced intentionally, as part of various
tradeoffs. That's what's going on here. I'm improving the correctness of the
system and avoiding pathological performance problems, while introducing a
performance regression that seems modest relative to the pathological
performance issue that's being mitigated.

s'marks

[1]
https://codeblog.jonskeet.uk/2010/07/29/there-s-a-hole-in-my-abstraction-dear-liza-dear-liza/

Alan

On May 4, 2020, at 5:25 PM, Stuart Marks <mailto:stuart.ma...@oracle.com>> wrote:

On 5/1/20 10:41 PM, Jason Mehrens wrote:
1. I assume you are using "c instanceof List" instead of "!(c instanceof
Set)" to correctly handle IdentitityHashMap.values()? The instanceof List
seems like safe choice but it is too bad we can still fool that check by
wrapping List as an unmodifiableCollection. If
splitIterator().characteristics() could tell us that the collection used
identity comparisons I think we would be able to determine if it was safe to
swap the ordering in the general case as we could check for IDENTITY, SORTED,
and comparator equality.

I'm using "instance List", not for the reason of IdentityHashMap.values()
specifically (though that's a good example), but mainly to try to be minimal.
While I think getting the semantics right takes priority, the change does
impact performance, so I want to reintroduce the performance heuristic in the
safest manner possible. Checking for !Set seems dangerous, as there might be
any number of Collections out there that aren't Sets and that aren't
consistent with equals. Checking for instanceof List s

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-08 Thread Stuart Marks

On 5/6/20 5:05 PM, Alan Snyder wrote:
Let me clarify. I was referring to the non-support by the framework, which
restricts membership semantics:

...[specification text regarding equals()]...

OK, thanks for the clarification.

Although the framework provides implementation classes that MAY be used to
create nonconforming collections, these classes are called out as potentially
non-conforming. For example:

[IdentityHashMap]

And in many places, the implications of non-conformance are also called out:

The phrase “affects the behavior” is open ended. It means all bets are off.

No, all bets are not off.

"All bets are off" isn't an unreasonable interpretation of the current wording
of the specification, but in fact the specification is incredibly poorly worded
and confusing. Plus there are several out-and-out bugs in the spec.

It's also somewhat odd to say that some collection implementations are
conforming whereas others aren't. Implementations can have bugs that render them
non-conformant to a specification. In this case, though, parts of the
specification contradict each other. You could pick part A of the spec and say
that part B is "non-conformant" but that really doesn't make sense if you
consider the totality of the specification. The fact is that the spec as a whole
is self-inconsistent. I intend to fix that.

My point is that (fully) supporting custom membership semantics changes a
fundamental property of the original design and should be approached as a
redesign. It is not a matter of fixing a series of issues one at a time, as you
discover them. Calling these issues semantic bugs when there is no violation of
the specification is leading you down the wrong path, in my opinion.

The issue of membership semantics is part of the original design.

If you look at the history, the collections framework was added in JDK 1.2, and
the JDK 1.2 specification includes both Set and SortedSet. There is the wording
in the SortedSet specification that is very similar to what exists today:
[edited for brevity]

Note that the ordering maintained by a sorted set must be consistent with
equals if the sorted set is to correctly implement the Set interface.
This is so because the Set interface is defined in terms of the equals
operation, but a sorted set performs all element comparisons using its
compareTo (or compare) method. The behavior of a sorted set is well-defined
even if its ordering is inconsistent with equals; it just fails to obey the
general contract of the Set interface.

The original designers (including Josh Bloch, and probably others) clearly
thought about these issues enough to put that wording in the specification. I
don't think they thought through the full implications of this, or at least they
didn't write it down, hence the vague wording above.

In addition, the issue was forgotten, and over time, bugs were introduced in the
system that made things worse. For example, the "optimization" that is the root
cause of the bug we are discussing (JDK-6394757) was introduced in JDK 1.3. In
the comments on this bug report [1] Bloch is quoted as saying

The spec clearly states that removeAll "Removes all this collection's
elements that are also contained in the specified collection." So the
current behavior is not just unpredictable, it's broken. It worked until
1.3, and was broken in 1.3-1.5. Sigh...

(And indeed, AbstractCollection.removeAll does precisely what the
Collection.removeAll spec demands)

[1]
https://bugs.openjdk.java.net/browse/JDK-6394757?focusedCommentId=12242803&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-12242803

Another bug, a spec bug this time, was introduced later. (I haven't searched the
history to find out exactly when.) The TreeSet.add() method specification says
this: [2]

Adds the specified element to this set if it is not already present. More
formally, adds the specified element e to this set if the set contains no
element e2 such that Objects.equals(e, e2). If this set already contains the
element, the call leaves the set unchanged and returns false.

[2]
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/util/TreeSet.html#add(E

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-12 Thread Stuart Marks





On 5/8/20 6:46 PM, Jason Mehrens wrote:

I assume Deque/Queue would suffer the same issue as List.  It would be nice to 
some how ensure ArrayDeque.contains is not called as far as the heuristic goes. 
 Looks like PriorityQueue uses equals for contains but that is not to say there 
are not other queue implementations in the wild that do something different.


The difficulty here is how far to take the heuristic. The goal here is not to 
guarantee that behavior can never be O(M*N) but to catch what seem to be the 
most common cases. I think the most common case is where the argument is a List, 
but there are lots of possibilities that we'd inevitably miss.



The more I think about it in general, it seems like your task would be easier 
if you could declare:
1. AbstractCollection.removeAll should assume this.contains is O(N) and iterate 
over this and check contains on arg.
2. AbstractSet.removeAll should iterate over argument and assume that 
this.contains/remove is at least O(log (N)) and assume argument.contains is 
O(N).
3. Concrete implementations of removeAll know the cost of their contains 
method.  If it is known to be O(N) then walk this otherwise walk the arg.
4. Include an example in removeAll that says if membership semantics differ between 
sets use: source.removeIf(e -> removals.contains(e)); or removals.forEach(e -> 
source.remove(e)); instead.  If they don't differ then use removeAll.


This sort of makes sense from a performance standpoint, but it doesn't address 
the semantic issues I raised in my reply the other day to Jens Lideström. 
Specifically, we don't want Set.removeAll or AbstractSet.removeAll to disagree 
with Collection.removeAll and AbstractCollection.removeAll. We also want 
removeAll() to be the complement of retainAll().



Given this has been around since JDK 1.3 would it be safe to assume that code 
that is mixing collections with different membership semantics is already 
working around this issue and therefore the risk is a bit lower in changing the 
spec?  Lots of concrete implementations already override removeAll anyway.


I'm not sure that's true. People keep running into either the performance 
problem or the semantic problem. Sometimes people live with bugs for a long time 
and can't figure it out. "Yeah, removeAll sometimes gives this weird result. I 
don't know why. Maybe I just don't understand Java."


s'marks

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-12 Thread Stuart Marks


You say:

The issue of membership semantics is part of the original design.

I agree, but only to the point of identifying inconsistent membership semantics 
as a source of non-conformance. What is not part of the original design is 
allowing a broader range of membership semantics to be conformant, which I 
assume is what you want.


I'm not using the concept of "conformance" so that's unrelated to what I want.


Another point of disagreement involves the specification of membership, 
especially in the case of Set, which I believe is where the problems arise.

I’m not completely sure what you are thinking, but it sounds like you believe 
that membership is defined by the implementation of the contains() method. I 
believe that is incorrect (i.e., not implied by the current specification).


No, I don't believe that membership is defined by the implementation of 
contains(). I'm not sure where that came from; it might be misapprehension or 
miscommunication.



Consider the “more formal” definition of contains():

More formally, returns true if and only if this set contains an element e 
such that Objects.equals(o, e).

Clearly, it would make no sense to define the contains() method in terms of 
itself. But it does make sense to say that the current state of a Set is a 
mathematical set of instances such that no pair of instances returns true from 
e1.equals(e2). I believe that is what the specification is trying to say, 
however imperfectly.


Yes, this much makes sense. Note that this embodies the membership contract of 
typical equals-based sets. Unfortunately, other sets (such as SortedSet) make 
some statements that imply that membership is determined by the comparator, they 
don't make any statments about how this affects contains(). The specification 
for SortedSet.contains(o) should say,


Returns true if and only if this set contains an element e such that
compare(o, e) == 0.

That it doesn't is yet another spec bug.


Consider the classic example, a TreeSet of Strings that is case-insensitive. If 
I add one string “hello” to an empty TreeSet, how many elements does it 
contain? The specification of add() says one. The size() method returns one. 
The iterator returns one element. But the contains() method returns true for 
multiple non-equal objects. Oops.


The reason this seems wrong is that you're calling contains() on two "non-equal" 
objects, but your statement's use of "non-equal" means you're using a different 
notion of equivalence from that used by the TreeSet. Since the TreeSet from your 
example uses case-insensitivity, those multiple "non-equal" objects on which 
you're calling contains() are in fact equivalent, so the result makes sense.



What I conclude from this exercise:


Does this mean you're close to conclusing this argument? :-)

I'll call out just one of your conclusions, since the other seem to be based on 
an incorrect assumption that I believe that the membership semantics are defined 
by the implementation of the contains() method, or that I'm proposing to do so. 
That conclusion is:



The problems of non-conforming collections are larger than you think. It is 
*not* the case that all of the basic axioms still apply.



Well I don't know how you know how large I think the problem is, but I'll agree 
it's pretty large. It's certainly much larger than the changeset that is 
currently under review.


Regarding the basic axioms, I'll return to some wording from SortedSet:

The behavior of a sorted set is well-defined even if its ordering is
inconsistent with equals; it just fails to obey the general contract
of the Set interface.

The "well-defined" clause means that the basic axioms apply.

However, I'll admit that the current wording of the spec does not support this! 
I'm assuming that there will be future changes to SortedSet et. al. that 
strengthen its notion of membership contract differing from other sets, and 
further that its add(), contains(), remove(), etc. methods all need to be 
adjusted to use this different membership contract.


Sorry if this wasn't clear. I know I've said this elsewhere, but possibly not on 
this thread or its predecessors.


s'marks

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-12 Thread Stuart Marks





On 5/9/20 7:48 PM, Alan Snyder wrote:

A small point… you might want to reconsider your analysis of Set.copyOf(), as 
it is a static method and there is no receiver.


Of course there is no receiver. For static factory methods, and for 
constructors, I meant the newly created instance is the one whose membership 
contract is used.


s'marks

Re: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

2020-05-12 Thread Stuart Marks





The containsAll() and equals() methods both use the membership contract of the 
receiver, not the argument. Unfortunately, the equals() specification says,


   Returns true if the specified object is also a set, the two sets have the
   same size, and every member of the specified set is contained in this set
   (or equivalently, every member of this set is contained in the specified
   set).

As should be clear from this discussion, the "equivalently" clause is 
incorrect -- another spec bug.


Changing Set.equals() in this way would make Set inconsistent with Object.
Do you really think that is a good idea?


[example of asymmetry of equals]

Your example illustrates that the "equivalently" clause is incorrect. I prefer 
specifications to have fewer incorrect statements.


s'marks

RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

2020-05-26 Thread Stuart Marks


Hi all,

Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
Activation for removal.


I'm listing this as S ("small") since conceptually it's fairly small, though 
there are rather a large number of files changed. Essentially the changes are:


 - java.rmi.activation package spec: add deprecation warning
 - java.rmi module spec: link to activation package spec
 - java.rmi.activation public classes and interfaces: deprecate
 - various other files: suppress warnings
 - Activation.java: have the rmid tool emit a deprecation warning
 - rmid.properties: localized warning message

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/

Bug ID:

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

JEP:

http://openjdk.java.net/jeps/385

Thanks,

s'marks

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

2020-05-27 Thread Stuart Marks





On 5/27/20 6:34 AM, Roger Riggs wrote:

rmid.properties: 134;  avoid breaking "have\n been" in to separate lines.
I would break after the ",".


Line break adjusted.


module-info.java: 35:  "version" -> "release" for consistency across the 
messages.

package-info.java: 41:  "version" -> "release" and "it may" -> "may" to be 
consistent

with the wording in other updates.


I switched to "version" everywhere since it's consistent with the wording added 
by javadoc for deprecated elements. For example,


https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/Compiler.html


Does this also need a CSR?  (There is one, 8245068, but its all boilerplate).


Yes, I'll work on the CSR next.

Thanks,

s'marks

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

2020-05-27 Thread Stuart Marks


Hi Lance, thanks for taking a look at this.

On 5/27/20 4:56 AM, Lance Andersen wrote:

I think this looks good.  I will add myself as a reviewer for the CSR.

I would probably create an issue for the release note and create a draft I 
believe I was asked for that when I was going through my CSR review for removal 
of the Java EE modules and CORBA.


I'll let you know when the CSR is ready for review. I'll start work on the 
release note too, good idea.


s'marks

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

2020-05-28 Thread Stuart Marks


I've updated the webrev a little bit in response to previous comments:

http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.1/

I've also drafted a CSR request and a release note. Please review:

CSR:

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

Release Note:

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

Thanks,

s'marks


On 5/26/20 12:35 PM, Stuart Marks wrote:

Hi all,

Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
Activation for removal.


I'm listing this as S ("small") since conceptually it's fairly small, though 
there are rather a large number of files changed. Essentially the changes are:


  - java.rmi.activation package spec: add deprecation warning
  - java.rmi module spec: link to activation package spec
  - java.rmi.activation public classes and interfaces: deprecate
  - various other files: suppress warnings
  - Activation.java: have the rmid tool emit a deprecation warning
  - rmid.properties: localized warning message

Webrev:

     http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/

Bug ID:

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

JEP:

     http://openjdk.java.net/jeps/385

Thanks,

s'marks

Re: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

2020-05-28 Thread Stuart Marks




On 5/28/20 12:13 PM, Lance Andersen wrote:

Thinking about:

-
@deprecated The RMI Activation mechanism has been deprecated, and it may
+ * be removed from a future version.


Perhaps it might be a bit clearer to say “… from a future Java SE version”?  I 
realize that is different from what is at: 
https://docs.oracle.com/en/java/javase/14/docs/api/java.base/java/lang/Compiler.html, 
but I thought it makes it clearer as to what “version” is referring to.


Though that being said,  and now looking back at what I did for the CORBA and 
Java EE module removal, I used the same wording as you are proposing,  so all 
good… :-)


Yeah, it does seem like it's missing a noun, doesn't it? Using "Java SE" could 
work, but I note that Mark R edited the JEP 385 text and changed the places 
where I had written "Java SE" to "the Java Platform", so I think I'll run with 
that wording.


Thanks for reviewing the CSR and the release note.

s'marks





I've also drafted a CSR request and a release note. Please review:

CSR:

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


I think the CSR is fine.  I might suggest adding a comment to justify “low” 
for the compatibility impact for the JCK folks.


I added myself as a reviewer.


Release Note:

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


Looks fine.

Best,

 Lance


Thanks,

s'marks


On 5/26/20 12:35 PM, Stuart Marks wrote:

Hi all,
Here's the implementation of the recently-posted JEP 385, deprecation of RMI 
Activation for removal.
I'm listing this as S ("small") since conceptually it's fairly small, though 
there are rather a large number of files changed. Essentially the changes are:

 - java.rmi.activation package spec: add deprecation warning
 - java.rmi module spec: link to activation package spec
 - java.rmi.activation public classes and interfaces: deprecate
 - various other files: suppress warnings
 - Activation.java: have the rmid tool emit a deprecation warning
 - rmid.properties: localized warning message
Webrev:
http://cr.openjdk.java.net/~smarks/reviews/8245068/webrev.0/
Bug ID:
https://bugs.openjdk.java.net/browse/JDK-8245068
JEP:
http://openjdk.java.net/jeps/385
Thanks,
s'marks


<http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif><http://oracle.com/us/design/oracle-email-sig-198324.gif>
<http://oracle.com/us/design/oracle-email-sig-198324.gif>Lance Andersen| 
Principal Member of Technical Staff | +1.781.442.2037

Oracle Java Engineering
1 Network Drive
Burlington, MA 01803
lance.ander...@oracle.com <mailto:lance.ander...@oracle.com>

Re: 8246183: Scanner/ScanTest.java fails due to SIGSEGV in StubRoutines::jshort_disjoint_arraycopy

2020-05-29 Thread Stuart Marks


OK, looks good. Thanks for jumping on this quickly.

s'marks

On 5/29/20 7:01 PM, Brian Burkhalter wrote:

Please review this fix [1] for [2]. It in effect just backs out the recent fix 
for [3]. I’ll investigate the root cause next week.

Thanks,

Brian

[1] http://cr.openjdk.java.net/~bpb/8246183/webrev.00/
[2] https://bugs.openjdk.java.net/browse/JDK-8246183
[3] https://bugs.openjdk.java.net/browse/JDK-8245121

Re: RFR: JDK-8230744 Several classes throw OutOfMemoryError without message

2020-06-02 Thread Stuart Marks





On 6/2/20 6:52 AM, Jim Laskey wrote:

Revised to reflect requested changes.

http://cr.openjdk.java.net/~jlaskey/8230744/webrev-01/index.html 



On this, if all you're doing is changing exception messages, then I don't care 
very much, modulo concerns about wording from others. If you start to get into 
changing the growth logic (like the Math.addExact stuff) then please see my 
message on the related thread, "Sometimes constraints are questionable."


Thanks,

s'marks

Re: Sometimes constraints are questionable

2020-06-02 Thread Stuart Marks


Hi Jim,

This was mentioned previously in this thread but not discussed very much. I 
suggest you take a look at jdk.internal.util.ArraysSupport.newLength(). Ivan 
Gerasimov and I worked this over fairly closely last year, and I'm pretty sure 
it does what Martin is saying, which I also think is the right thing.


The intent is that it be used for things that have growable arrays, where the 
array might have a larger capacity than the logical number of elements currently 
stored. Sometimes the array needs to be grown to accommodate an immediate need 
(minGrowth) but it's desired to grow larger (prefGrowth) in anticipation of 
future needs. If minGrowth can't be accommodated, it throws OOME, but if 
prefGrowth can't be accommodated, it might be acceptable to provide a smaller 
amount of growth.


(Of course, all this assumes that there is sufficient memory available to 
allocate the actual array. ArraysSupport.newLength doesn't attempt to ascertain 
that.)


One issue is integer wraparound (overflow). This is the primary value that 
ArraysSupport.newLength provides. It would be good to centralize these 
computations instead of having them be spread all over.


Another issue is the one that MAX_ARRAY_LENGTH (also called MAX_ARRAY_SIZE) is 
trying to address. This is sort-of a misnomer. It's not the actual maximum array 
size (which in fact isn't known the the library). It's actually the maximum 
array size that the library is fairly confident the VM can provide, assuming 
that enough memory is actually available. What the heck does that mean?


The theoretical maximum array size is Integer.MAX_VALUE, since the JLS and JVMS 
don't allow anything larger. However, actual VM implementations will refuse to 
allocate an array above a certain amount slightly smaller than that, even if 
there is enough memory available. In practice, I believe the values for current 
Hotspot are Integer.MAX_VALUE-3 or Integer.MAX_VALUE-2, depending on whether 
compressed OOPS are in use.


Why is this significant? Consider the following case, where the capacity of 
something is currently Integer.MAX_VALUE-100, and it's filled with elements. The 
application performs some operation that requires 50 elements (minGrowth) be 
added. A new array could certainly be allocated with size Integer.MAX_VALUE-50, 
but typical growth policies for these kinds of containers want to increase the 
current capacity by 1.5x or 2x (prefGrowth). Doing this multiplication would 
exceed Integer.MAX_VALUE, so that won't work. Clearly, we need to clamp the 
capacity somewhere.


We don't want to clamp the capacity at Integer.MAX_VALUE, because this 
allocation would fail on every JVM I'm aware of, even if enough memory is 
available. So we don't do that. Instead, we clamp the preferred growth at some 
fairly arbitrary value smaller than Integer.MAX_VALUE, which is here called 
MAX_ARRAY_LENGTH, and increase the capacity to that instead. This allows the 
container's requested allocation to proceed: it satisfies minGrowth, but it 
doesn't satisfy prefGrowth. Instead, it returns a capacity value that's 
reasonably likely to succeed, given an unknown JVM implementation limit.


Recall that the container now has Integer.MAX_VALUE-50 elements and the capacity 
is MAX_ARRAY_SIZE, which is currently defined somewhat arbitrarily at 
Integer.MAX_VALUE-8. Suppose the application now wants to add 43 elements. What 
should happen?


We could say, this exceeds MAX_ARRAY_LENGTH, so refuse the request and throw 
OOME. This is reasonable and consistent in some sense, but perhaps not in 
another. Suppose there is sufficient memory, and the JVM does allow arrays of 
Integer.MAX_VALUE-7 to be created. Shouldn't we even try?


That's what hugeLength() does -- it returns a value that attempts an allocation 
beyond the max preferential growth, and leaves it up to the JVM to grant or 
refuse the request based on its own implementation limits.


Anyway, this is all quite subtle, and maybe comments in ArraysSupport don't 
describe this adequately. But the code that implements this kind of policy has 
been copied to different locations around the JDK, and it uses somewhat 
different terminology, and it might have slightly different bugs, but they're 
all essentially trying to implement this policy.


**

Several questions could be asked:

1) Is this the right policy for implementing growable arrays?

2) In cases where a class needs a growable array, can and should it be 
refactored to use ArraysSupport.newLength()?


3) Does ArraysSupport.newLength() need to be modified to accommodate needs of 
additional call sites?


4) We might want to consider refactoring PriorityBlockingQueue and ArrayDeque to 
use ArraysSupport.newLength, in order to provide a consistent policy for 
collections. Other growable-array-based collections (Vector, ArrayList, 
PriorityQueue) do already.


s'marks





On 6/1/20 4:47 AM, Jim Laskey wrote:

Thanks David will run with that,



On May 31, 2020, at 8:34 PM

Re: Sometimes constraints are questionable

2020-06-03 Thread Stuart Marks


On 6/2/20 11:49 PM, David Holmes wrote:
IIUC what you are saying is that MAX_ARRAY_LENGTH is treated as a soft-limit. A 
request for prefGrowth won't be allowed to exceed it. But if minGrowth takes the 
length passed it then the code tries to do the allocation that large anyway. If 
it succeeds we win, and if we get OOME that is what we would have thrown anyway 
if we rejected the request as too big.


Yes, that's a good, succinct summary.

So my misunderstanding in this was that MAX_ARRAY_LENGTH is not attempting to 
define the actual VM hard limit, just a large value close to that which is 
expected to always be valid (actual memory permitting).


Exactly. And sorry for this misunderstanding -- revisiting this I see that the 
name MAX_ARRAY_LENGTH is quite misleading. Maybe I should have pushed harder to 
change this when Ivan and I went through this code last year. We had improved 
some things, I think, but we had already gone through half a dozen rounds of 
review and I think we were both getting tired. :-)


s'marks

Re: Sometimes constraints are questionable

2020-06-03 Thread Stuart Marks

ybe we should update the documentation of 
java.lang.OutOfMemoryError to mention this.


s'marks





Cheers,

-- Jim




On Jun 2, 2020, at 7:08 PM, Stuart Marks <mailto:stuart.ma...@oracle.com>> wrote:


Hi Jim,

This was mentioned previously in this thread but not discussed very much. I 
suggest you take a look at jdk.internal.util.ArraysSupport.newLength(). Ivan 
Gerasimov and I worked this over fairly closely last year, and I'm pretty sure 
it does what Martin is saying, which I also think is the right thing.


The intent is that it be used for things that have growable arrays, where the 
array might have a larger capacity than the logical number of elements 
currently stored. Sometimes the array needs to be grown to accommodate an 
immediate need (minGrowth) but it's desired to grow larger (prefGrowth) in 
anticipation of future needs. If minGrowth can't be accommodated, it throws 
OOME, but if prefGrowth can't be accommodated, it might be acceptable to 
provide a smaller amount of growth.


(Of course, all this assumes that there is sufficient memory available to 
allocate the actual array. ArraysSupport.newLength doesn't attempt to 
ascertain that.)


One issue is integer wraparound (overflow). This is the primary value that 
ArraysSupport.newLength provides. It would be good to centralize these 
computations instead of having them be spread all over.


Another issue is the one that MAX_ARRAY_LENGTH (also called MAX_ARRAY_SIZE) is 
trying to address. This is sort-of a misnomer. It's not the actual maximum 
array size (which in fact isn't known the the library). It's actually the 
maximum array size that the library is fairly confident the VM can provide, 
assuming that enough memory is actually available. What the heck does that mean?


The theoretical maximum array size is Integer.MAX_VALUE, since the JLS and 
JVMS don't allow anything larger. However, actual VM implementations will 
refuse to allocate an array above a certain amount slightly smaller than that, 
even if there is enough memory available. In practice, I believe the values 
for current Hotspot are Integer.MAX_VALUE-3 or Integer.MAX_VALUE-2, depending 
on whether compressed OOPS are in use.


Why is this significant? Consider the following case, where the capacity of 
something is currently Integer.MAX_VALUE-100, and it's filled with elements. 
The application performs some operation that requires 50 elements (minGrowth) 
be added. A new array could certainly be allocated with size 
Integer.MAX_VALUE-50, but typical growth policies for these kinds of 
containers want to increase the current capacity by 1.5x or 2x (prefGrowth). 
Doing this multiplication would exceed Integer.MAX_VALUE, so that won't work. 
Clearly, we need to clamp the capacity somewhere.


We don't want to clamp the capacity at Integer.MAX_VALUE, because this 
allocation would fail on every JVM I'm aware of, even if enough memory is 
available. So we don't do that. Instead, we clamp the preferred growth at some 
fairly arbitrary value smaller than Integer.MAX_VALUE, which is here called 
MAX_ARRAY_LENGTH, and increase the capacity to that instead. This allows the 
container's requested allocation to proceed: it satisfies minGrowth, but it 
doesn't satisfy prefGrowth. Instead, it returns a capacity value that's 
reasonably likely to succeed, given an unknown JVM implementation limit.


Recall that the container now has Integer.MAX_VALUE-50 elements and the 
capacity is MAX_ARRAY_SIZE, which is currently defined somewhat arbitrarily at 
Integer.MAX_VALUE-8. Suppose the application now wants to add 43 elements. 
What should happen?


We could say, this exceeds MAX_ARRAY_LENGTH, so refuse the request and throw 
OOME. This is reasonable and consistent in some sense, but perhaps not in 
another. Suppose there is sufficient memory, and the JVM does allow arrays of 
Integer.MAX_VALUE-7 to be created. Shouldn't we even try?


That's what hugeLength() does -- it returns a value that attempts an 
allocation beyond the max preferential growth, and leaves it up to the JVM to 
grant or refuse the request based on its own implementation limits.


Anyway, this is all quite subtle, and maybe comments in ArraysSupport don't 
describe this adequately. But the code that implements this kind of policy has 
been copied to different locations around the JDK, and it uses somewhat 
different terminology, and it might have slightly different bugs, but they're 
all essentially trying to implement this policy.


**

Several questions could be asked:

1) Is this the right policy for implementing growable arrays?

2) In cases where a class needs a growable array, can and should it be 
refactored to use ArraysSupport.newLength()?


3) Does ArraysSupport.newLength() need to be modified to accommodate needs of 
additional call sites?


4) We might want to consider refactoring PriorityB

Re: Sometimes constraints are questionable

2020-06-05 Thread Stuart Marks





On 6/3/20 10:36 AM, Stuart Marks wrote:
3) Integer wraparound/overflow during size computation. AS.newLength generates 
this:


     OutOfMemoryError: Required array length too large

(3) is the only case generated by the library. In fact, AS.hugeLength() has 
oldLength and minGrowth parameters, which seems like enough detail already. 
These could reasonably be formatted into the error message, something like:


     private static int hugeLength(int oldLength, int minGrowth) {
     int minLength = oldLength + minGrowth;
     if (minLength < 0) { // overflow
     throw new OutOfMemoryError(
     String.format("Required array length %d + %d too large", 
oldLength, minGrowth));

     }

Would this help? If this were added, would it be sufficient to allow various use 
sites to convert to use AS.newLength? (Except possibly StringJoiner.)


Anything further on this? Should I file a bug/rfe for this? Also, I could update 
the docs to explain ArraysSupport.newLength better, per my earlier exchange with 
David Holmes.


s'marks

Re: Final call Re: RFR: JDK-8230744 Several classes throw OutOfMemoryError without message

2020-06-10 Thread Stuart Marks

Regarding the wording, I don't think either "implementation limit" or "VM limit" 
is correct, at least not in all the cases.


If we were using ArraysSupport.newLength, the error messages are as follows:


1) Actual shortage of heap space. Hotspot generates this:

OutOfMemoryError: Java heap space

2) Requested array size exceeds implementation limit. Hotspot
generates this:

OutOfMemoryError: Requested array size exceeds VM limit

3) Integer wraparound/overflow during size computation.
ArraysSupport.newLength() generates this:

OutOfMemoryError: Required array length too large


For this changeset, we're leaving several places as they are, either using the 
*exact arithmetic methods or doing their own checks. OK, we'll save refactoring 
to ArraysSupport.newLength for another time. But the resulting OOME message is I 
think misleading.


For example, in String.java line 2190, we catch an exception from Math.addExact 
or Math.multiplyExact. This occurs if the arithmetic result exceeds the range of 
an int. It's less about some implementation or VM limit, but instead the 
requested length simply cannot be represented by an int.


Also, while I'm not as allergic to talking about arrays as Martin, in this case 
the subsequent code allocates a StringBuilder and not an array. So it is 
somewhat odd for this message to refer to an array.


Anyway, instead of

Requested array size exceeds VM limit

I'd suggest this:

Requested size too large

(Or "length" instead of "size". "Length" is more consistent for arrays and 
strings, but oddly enough, Hotspot uses "array size".)


I think this wording could apply to String.java, StringLatin1.java,
StringUTF16.java, Pattern.java, UnsyncByteArrayOutputStream.java, and 
ByteArrayChannel.java. I guess we could try to provide a customized message for 
each area, but that wouldn't seem to me to add much value.


I'm not entirely sure what's going on in AbstractStringBuilder.java. It uses 
ArraysSupport.newLength(), which should either return an in-range value or 
throw. I don't understand why the code in ASB.newCapacity() is checking length 
against Integer.MAX_VALUE and throwing again. This shouldn't happen by the spec 
of AS.newLength(). I think that check (lines 258-259) can simply be removed.


s'marks

On 6/10/20 10:22 AM, Roger Riggs wrote:

Hi Jim,

In Unsafe.java: 632: the "Unable to allocate 98494837395: doesn't read very 
well.
Please add " bytes" to the end.

I would probably drop "array" from all the messages but at least in the String* 
cases.

As Martin notes, the array is an implementation detail.
(And i still prefer "implementation limit" over "VM limit".

Thanks, Roger



On 6/10/20 12:33 PM, Jim Laskey wrote:



On Jun 130, 2020, at 1:15 PM, Martin Buchholz  wrote:

I took a look at PriorityBlockingQueue.

Part of converting to ArraysSupport involves deleting the local orphan
MAX_ARRAY_SIZE; that needs to be done.

Removed.


---
It looks like
newCap > oldCap
is always true, so we should delete the test?
 if (newCap > oldCap && queue == array)
---

If oldCap == MAX_ARRAY_SIZE wouldn't newCap == oldCap



In Pattern.java I see
+    throw new OutOfMemoryError("Requested array size exceeds
VM limit");

That wording doesn't seem useful to me - the use of an array is an
implementation detail, and the user didn't __request__ it.

Better seems the wording in ArraysSupport
    throw new OutOfMemoryError("Required array length too large");
but if we're going to the trouble of composing a custom detail
message, I'd try harder to find one meaningful to the user of the API,
something like "pattern too large"

Done and done.

Thank you.




On Wed, Jun 10, 2020 at 5:15 AM Jim Laskey  wrote:

Will push if no comments by EOB.


On Jun 8, 2020, at 2:22 PM, Jim Laskey  wrote:

Revised to use a consistent error message. Modified AbstractStringBuilder 
and PriorityBlockingQueue to use ArraysSupport.newLength(). Didn't modify 
ByteArrayChannel and UnsyncByteArrayOutputStream since those changes would 
require changes to module exporting.


webrev: http://cr.openjdk.java.net/~jlaskey/8230744/webrev-03/index.html 
<http://cr.openjdk.java.net/~jlaskey/8230744/webrev-03/index.html>




On Jun 3, 2020, at 11:24 AM, Jim Laskey  wrote:

It's not the goal or role of this bug to fix the wrongs of the past, 
merely add error messages to the exceptions. I raised the discussion as an 
issue. Clearly there is a correct path to follow. If you think more effort 
is required then file a bug. :-)


Cheers,

-- Jim





On Jun 2, 2020, at 7:13 PM, Stuart Marks  wrote:



On 6/2/20 6:52 AM, Jim Laskey wrote:

Revised to reflect requested changes.
http://cr.openjdk.java.net/~

Re: Sometimes constraints are questionable

2020-06-10 Thread Stuart Marks


On 6/5/20 1:48 PM, James Laskey wrote:

I’m fixing the two in java.base. The other two are in different modules and 
would require changes to export. So you can file on those.


I've filed

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

to enhance the exception message, docs, and to add a test for 
ArraysSupport.newLength.


There's a separate discussion of whether various things in the JDK should be 
refactored to use this utility method. ArraysSupport is in jdk.internal.util, 
which is not exported, and I'm fairly reluctant to add a qualified export just 
for this. So perhaps things outside of java.base can live with doing their own 
calculations.


For other cases, let's wait until after your OOME message fix (JDK-8230744) goes 
in, since it seems like it might take care of a couple right away. Then we can 
revisit the various cases and file bugs individually for them. In particular, 
the use of Math.addExact et. al. and catch/rethrow of ArithmeticException can 
probably be replaced with a call to ArraysSupport.newLength, but individual 
cases will have to be looked at carefully.


s'marks

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

2020-06-26 Thread Stuart Marks





On 6/25/20 4:53 PM, Joe Wang wrote:
Please review a fix to a BCEL regression. At issue was the addition of 
hashCode() method to Instruction.java in BCEL 6.0. This hashCode() method was 
implemented to return the instruction's opcode that unfortunately is mutable. 
The problem hasn't showed up until the code path led to calls to remove from a 
HashSet a target that has been changed since it was added to the HashSet. The 
proposed patch is to make the hashCode final/immutable.


This patch implies that a target object is considered the same one even if its 
field values may have been changed. It therefore may not be appropriate in other 
situations (or may cause problems). However, since it had always had no 
hashCode() override before BCEL 6.0, thereby relying on Object's identity hash 
code, its use case has been limited and time-tested. It can benefit from this 
patch in that it provides the same function as Object's hash code, and then 
serves as a reminder (to any who might read the code) how it was used (objects 
considered to be the same over the course as far as the hashCode is concerned).


JBS: https://bugs.openjdk.java.net/browse/JDK-8248348
webrevs: http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev/


Hi Joe,

This fix seems vaguely wrong, but in the context of what I understand it's 
doing, it seems that BCEL or the surrounding JAXP code is even more wrong. :-)


From what I understand, this bugfix was prompted by a case where an Instruction 
was added to a HashSet. The Instruction was then mutated while in the HashSet, 
and this resulted in it not being removed from the HashSet in the future, when 
it was necessary to do so, thus leaving a stale entry. How could this have 
possibly worked?!?


I'm speculating a bit, but I could imagine code like this:

var inst = new Instruction(ORIG_OPCODE);
var set = new HashSet();
set.add(inst);
...
inst.setOpcode(NEW_OPCODE); // (1) mutates inst while in HashSet!
...
set.remove(inst); // (2)

In the current version, the hashCode() is simply the opcode, and equals() 
compares opcodes (and possibly other things). This is correct from the 
standpoint of Instruction itself, as equal items have the same hashcode. 
However, the above code fails because at line (2), the remove() method looks in 
the "wrong" hash bucket and can't find the instruction, so it does nothing.


The patch changes the class to compute the hashcode at construction time, and 
makes the hashcode constant, so it "fixes" the above code, since the remove() 
method will look in the same bucket and find the desired instance. But now 
Instruction breaks the hashCode/equals contract, since it's possible for two 
instances to be equal but to have different hashcodes. (Consider an Instruction 
that was created with NEW_OPCODE. It can be equal to inst, but its hashcode will 
differ.)


It seems to me that this patch is compensating for broken code elsewhere in BCEL 
or JAXP by adding more brokenness. It could be that is ok in this narrow usage 
context. That is, as far as we know, this "shaded" copy of BCEL is used *only* 
by JAXP and not used in general, and it may be that the particular ways that 
JAXP uses BCEL aren't broken (and are indeed fixed) by this change. But I'd like 
to see some assurance of that. It could be that there are bugs introduced by 
this change that simply haven't been uncovered by the testing we've done yet.


Another approach is to figure out what is mutating the Instruction while it's in 
a HashSet and fix that instead.


Unfortunately, either of these approaches seems to involve an arbitrary amount 
of work. :-(


s'marks

Re: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

2020-06-30 Thread Stuart Marks




An unconventional patch over an unusual hashcode/equals implementation is a bit 
too controversial. I'd like to propose a new patch that focus on the problem at 
hand, that is re-setting the opcode causes the item in HashSet to get lost 
because of the changed hash bucket. This patch therefore simply remove it from 
the HashSet before the change and then add it back as if it's a new item after 
setting the opcode. This patch resolves the issue while leaves BCEL 6.0's 
hashCode/equals alone.


Here's the new patch:
http://cr.openjdk.java.net/~joehw/jdk16/8248348/webrev02/


Hi Joe,

This makes a good deal more sense than the previous approach! As you note this 
preserves the consistency of equals() and hashCode(), which is an important 
invariant.


It seems a bit roundabout to have to remove something from the HashSet, mutate 
it, and re-add it, but I don't see an alternative. Mutating it while in the 
HashSet is the original cause of problem. Trying to contort things so that 
objects can remain in the HashSet while being mutated is difficult in general, 
and it seems likely to lead to additional problems. Given this, removing and 
re-adding seems the most sensible approach.


It's a bit fragile in that BranchInstruction now has to "know" that the 
InstructionTarget has a HashSet that requires this to be done. However, I don't 
see a way to avoid this without redesigning the relationships of the objects in 
this area -- which seems out of scope for this change.


Just one comment on the test: there are large test data files 
BCELHashCodeTest1.xsl and BCELHashCodeTest2.xsl. What are these? I'm not asking 
for a tutorial, but rather about what function they serve relative to this bug. 
Just a sentence or two about how BCELHashCodeTest2.xsl causes (I think) the 
generated bytecode to exceed the 64Kbyte method size limit. Splitting this into 
multiple methods requires mutating some of the instructions, and then hilarity 
ensued. :-)


I'm also not sure that BCELHashCodeTest1.xsl is necessary if it passes both with 
and without the patch. But if you think it's valuable, then it's ok for it to 
stay in.


Aside from this, I think this change is good to go.


(had a nice hike today, and asked the beautiful Lake 22 ;-))


I had to look it up. Lake 22 looks very nice! Please give my regards the next 
time you visit.


s'marks

Re: RFR [15/java.xml] 8248884: Regression caused by the update to BCEL 6.0

2020-07-06 Thread Stuart Marks





On 7/6/20 4:37 PM, huizhe.w...@oracle.com wrote:
Please refer to the previous review for 8248348. This patch should have been 
pushed into 15 in the first place. I hope a merge with 16 repo won't cause any 
trouble/conflict.


JBS: https://bugs.openjdk.java.net/browse/JDK-8248884

webrev: http://cr.openjdk.java.net/~joehw/jdk15/8248884/webrev/


OK, looks good. Remember to push the backport changeset using the main bug ID, 
which in this case is JDK-8248348.


Also, we usually rely on the hgupdater to create the backport record 
automatically. You've already created backport record JDK-8248884, so just leave 
it there in case the hgupdater reuses it. However, if for some reason hgupdater 
decides to create a *new* backport, then close this one out as Not An Issue or 
something.


Thanks,

s'marks

Re: JDK 16 RFR of 8250580: Address reliance on default constructors in java.rmi

2020-07-28 Thread Stuart Marks


Looks good Joe, thanks for doing this cleanup. CSR request reviewed.

s'marks

On 7/24/20 7:45 PM, Joe Darcy wrote:

Hello,

Another bug in the quest to remove use of default constructors in the JDK's 
public API, this time in the java.rmi module:


     webrev: http://cr.openjdk.java.nhet/~darcy/8250580.0/
     CSR: https://bugs.openjdk.java.net/browse/JDK-8250581

Patch below; thanks,

-Joe

--- old/src/java.rmi/share/classes/java/rmi/server/RMIClassLoaderSpi.java 
2020-07-24 19:42:16.353847343 -0700
+++ new/src/java.rmi/share/classes/java/rmi/server/RMIClassLoaderSpi.java 
2020-07-24 19:42:15.645847343 -0700

@@ -1,5 +1,5 @@
  /*
- * Copyright (c) 2000, 2006, Oracle and/or its affiliates. All rights reserved.
+ * Copyright (c) 2000, 2020, Oracle and/or its affiliates. All rights reserved.
   * DO NOT ALTER OR REMOVE COPYRIGHT NOTICES OR THIS FILE HEADER.
   *
   * This code is free software; you can redistribute it and/or modify it
@@ -62,6 +62,11 @@
  public abstract class RMIClassLoaderSpi {

  /**
+ * Constructor for subclasses to call.
+ */
+    public RMIClassLoaderSpi() {}
+
+    /**
   * Provides the implementation for
   * {@link RMIClassLoader#loadClass(URL,String)},
   * {@link RMIClassLoader#loadClass(String,String)}, and

Re: RFR: 8252830: Correct missing javadoc comments in java.rmi module [v3]

2020-09-08 Thread Stuart Marks

On Tue, 8 Sep 2020 19:44:21 GMT, Roger Riggs  wrote:

>> 8252830: Correct missing javadoc comments in java.rmi module
>
> Roger Riggs has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   Added java.io.Serial to java.rmi.activation.ActivationID

Marked as reviewed by smarks (Reviewer).

src/java.rmi/share/classes/java/rmi/server/RemoteObject.java line 368:

> 366: @java.io.Serial
> 367: private void writeObject(java.io.ObjectOutputStream out)
> 368: throws java.io.IOException

It's a little odd to see an import of java.io.IOException (which is used by the 
`@throws` tag) but not to have it used
here. There's another occurrence down below. These files have a mixture of 
imports and fully qualified names, which I
find irritating, which might be nice to clean up. But you might find beyond the 
scope of this change. So, clean up as
much of this as you like, or none if you prefer, no big deal.

-

PR: https://git.openjdk.java.net/jdk/pull/79

RFR: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types

2020-09-08 Thread Stuart Marks

Add some generics and wrap in `{@code}` to protect angle brackets.

-

Commit messages:
 - 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types

Changes: https://git.openjdk.java.net/jdk/pull/87/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=87&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8157729
  Stats: 8 lines in 2 files changed: 0 ins; 0 del; 8 mod
  Patch: https://git.openjdk.java.net/jdk/pull/87.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/87/head:pull/87

PR: https://git.openjdk.java.net/jdk/pull/87

Re: RFR: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types [v2]

2020-09-08 Thread Stuart Marks

On Tue, 8 Sep 2020 22:58:58 GMT, Lance Andersen  wrote:

>> Stuart Marks has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Update copyright years.
>
> Marked as reviewed by lancea (Reviewer).

Aha, I knew I forgot something! The copyright years.

Actually, I did that on purpose...to practice updating an existing PR. Heh, 
heh, heh.

BTW I believe this does not require a CSR since this is merely example code.

Thanks for the reviews.

-

PR: https://git.openjdk.java.net/jdk/pull/87

Re: RFR: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types [v2]

2020-09-08 Thread Stuart Marks

> Add some generics and wrap in `{@code}` to protect angle brackets.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Update copyright years.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/87/files
  - new: https://git.openjdk.java.net/jdk/pull/87/files/b40485ba..527d8a16

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=87&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=87&range=00-01

  Stats: 2 lines in 2 files changed: 0 ins; 0 del; 2 mod
  Patch: https://git.openjdk.java.net/jdk/pull/87.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/87/head:pull/87

PR: https://git.openjdk.java.net/jdk/pull/87

Integrated: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types

2020-09-08 Thread Stuart Marks

On Tue, 8 Sep 2020 22:32:47 GMT, Stuart Marks  wrote:

> Add some generics and wrap in `{@code}` to protect angle brackets.

This pull request has now been integrated.

Changeset: 30fa8d5d
Author:    Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/30fa8d5d
Stats: 10 lines in 2 files changed: 0 ins; 0 del; 10 mod

8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types

Reviewed-by: darcy, naoto, lancea

-

PR: https://git.openjdk.java.net/jdk/pull/87

Re: RFR: 8251495: Clarify DOM documentation

2020-09-09 Thread Stuart Marks

On Wed, 9 Sep 2020 22:56:14 GMT, Joe Wang  wrote:

> Revert changes made by JDK-8249643, removing the implNote.

Marked as reviewed by smarks (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/100

RFR: 8253066: typo in Stream.mapMulti

2020-09-11 Thread Stuart Marks

A simple typo fix.

-

Commit messages:
 - fix typo in Stream.mapMulti

Changes: https://git.openjdk.java.net/jdk/pull/137/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=137&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8253066
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/137.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/137/head:pull/137

PR: https://git.openjdk.java.net/jdk/pull/137

Integrated: 8253066: typo in Stream.mapMulti

2020-09-11 Thread Stuart Marks

On Fri, 11 Sep 2020 22:20:48 GMT, Stuart Marks  wrote:

> A simple typo fix.

This pull request has now been integrated.

Changeset: b1b0f0b2
Author:    Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/b1b0f0b2
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8253066: typo in Stream.mapMulti

Reviewed-by: darcy, lancea

-

PR: https://git.openjdk.java.net/jdk/pull/137

Re: RFR: 8252730: jlink does not create reproducible builds on different servers

2020-09-14 Thread Stuart Marks

On Mon, 14 Sep 2020 20:35:24 GMT, Ian Graves  wrote:

> Related to [JDK-8252730 jlink does not create reproducible builds on different
> servers](https://bugs.openjdk.java.net/browse/JDK-8252730). Introduces 
> ordering based on `Archive` module names to
> ensure stable files (and file signatures) across generated JDK images by 
> jlink.

Sorting the Archive instances makes more sense to me than trying to add 
hashCode/equals to Archive. I note that the
instances added to the ResourcePoolManager are stored in a LinkedHashMap, so 
the sorted order should be preserved. I'll
defer to Alan and Jim as to whether sorting by module name is the right thing. 
Additional code comments to follow.

src/jdk.jlink/share/classes/jdk/tools/jlink/internal/ImageFileCreator.java line 
274:

> 272: e.getResourcePoolEntryName(), e))
> 273: .forEach(resources::add);
> 274: });

I'm somewhat allergic to nested forEach plus statement lambdas. I note that the 
original code mixed an imperative
for-loop with a stream, which was kind of odd; converting to a straight nested 
for-loop might be reasonable.
Alternatively, the nested forEaches could be converted to a flatMap as follows:

`
archives.stream()
.map(Archive::moduleName)
.sorted()
.flatMap(mn -> entriesForModule.get(mn).stream()
   .map(e -> new ArchiveEntryResourcePoolEntry(mn, 
e.getResourcePoolEntryName(), e)))
.forEach(resources::add);
`

-

PR: https://git.openjdk.java.net/jdk/pull/156

Re: 'Find' method for Iterable

2020-09-16 Thread Stuart Marks





On 9/16/20 1:59 PM, Remi Forax wrote:

- Mail original -

De: "Nir Lisker" 
À: "core-libs-dev" 
Envoyé: Lundi 14 Septembre 2020 20:56:27
Objet: 'Find' method for Iterable



Hi,

This has probably been brought up at some point. When we need to find an
item in a collection based on its properties, we can either do it in a
loop, testing each item, or in a stream with filter and findFirst/Any.

I would think that a method in Iterable be useful, along the lines of:

public  Optional find(Predicate condition) {
Objects.requireNonNull(condition);
for (T t : this) {
 if (condition.test(t)) {
 return Optional.of(t);
}
}
return Optional.empty();
}

With usage:

list.find(person -> person.id == 123456);

There are a few issues with the method here such as t being null in
null-friendly collections and the lack of bound generic types, but this
example is just used to explain the intention.

It will be an alternative to

list.stream().filter(person -> person.id == 123456).findAny/First()
(depending on if the collection is ordered or not)

which doesn't create a stream, similar to Iterable#forEach vs
Stream#forEach.

Maybe with pattern matching this would become more appetizing.


During the development of Java 8, we first tried to use Iterator/Iterable 
instead of using a novel interface Stream.
But a Stream cleanly separate the lazy side effect free API from the mutable 
one (Collection) and can be optimized better by the VM (it's a push API instead 
of being a pull API).

The other question is why there is no method find() on Collection, i believe 
it's because while find() is ok for any DB API, find() is dangerous on a 
Collection because the execution time is linear, so people may use it instead 
of using a Map.



Hi Nir,

Rémi is correct to point out this distinction between the lazy operations (which 
appear on Stream) and the eager (and possibly mutating) operations on Collections. I 
think we want to preserve this distinction.


While it might not be difficult to add a find() method to Iterable, why limit it to 
the find operation, and what about all the other operations available on Stream? 
Maybe what's necessary is a way to convert an Iterable to a Stream. In fact, this is 
already possible:


StreamSupport.stream(iterable.spliterator(), false)

Well, this is mouthful, so maybe there ought to be an easier way to convert an 
Iterable to a Stream.


On the other hand, your examples use a list. The List interface already has methods 
indexOf/lastIndexOf which search the list for a particular object that's compared 
using equals(). It seems reasonable to consider similar methods that take a 
predicate instead of an object.


Does either of these sound promising?

s'marks

Re: RFR: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

2020-09-17 Thread Stuart Marks

On Fri, 11 Sep 2020 15:17:58 GMT, Bradford Wetmore  wrote:

>> Ok, sorry for the distraction.
>
> Our local Santuario maintainer says:
> 
> In general, changes to Apache Santuario should also be made at Apache so we 
> stay in sync.

Hi @doom369, I hope we didn't end up wasting too much of your time with this. I 
wanted to respond to a comment you made
earlier in this PR,

> I have in mind dozens of improvements all over the code like this one.

It's hard to see, but as you discovered, the JDK has different groups of people 
maintaining different areas, and
sometimes there are hidden constraints on those different areas, for example, 
to avoid divergence with upstream source
bases. And as you discovered, sometimes those source bases might need to 
maintain compatibility with an older JDK ...
so we don't want to update this code even though it's IN the JDK.

Those kind of constraints generally don't apply to code in the java.base 
module, since there's nothing upstream of it,
and by definition it cannot depend on anything outside the JDK. Generally -- 
though there are exceptions -- we're more
receptive to keeping stuff in java.base (and sometimes related modules close to 
the core) on the latest and greatest
stuff. There are some constraints, however. There are some things we can't use 
too early during initialization of the
JDK, such as lambdas. Also, there is some code lurking around that is sometimes 
executed by the boot JDK, which is
typically one release behind. (This is definitely the case for tools like 
javac, but I think it might also apply to
some things in base.)

Anyway, if you'd like to pursue some of these improvements, drop a note to 
core-libs-dev@openjdk and we can talk about
it.

Thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/29

Re: RFR 8253451: Performance regression in java.util.Scanner after 8236201

2020-09-22 Thread Stuart Marks


Hi Martin,

Overall it seems reasonable to replace the \x{HH} construct with a simpler escape 
sequence. I'm a bit surprised to see the performance impact, but you noticed it, so 
I'll buy that it's significant.



 // These must be literalized to avoid collision with regex
 // metacharacters such as dot or parenthesis
-groupSeparator =   "\\x{" + Integer.toHexString(dfs.getGroupingSeparator()) + 
"}";
-decimalSeparator = "\\x{" + Integer.toHexString(dfs.getDecimalSeparator()) + 
"}";
+char newSep;
+groupSeparator = ((newSep = dfs.getGroupingSeparator()) == ',' ||
+newSep == '.' || newSep == ' ' ? "\\" + newSep :
+"\\x{" + Integer.toHexString(newSep) + "}" );
+decimalSeparator = ((newSep = dfs.getDecimalSeparator()) == '.' ||
+newSep == ',' ? "\\" + newSep : "\\x{" +
+Integer.toHexString(newSep) + "}" );


Fundamentally it's simple, but there's rather a lot going on here:

 - reuse of a local variable
 - assignment within a logical expression
 - odd line breaking
 - ternary operator

These factors make the code rather difficult to read, unnecessarily so in my 
opinion.

(The collections and concurrency libraries sometimes put assignments within other 
expressions, but usually to avoid unnecessary initialization of a local variable. I 
don't think that applies here.)


More to the semantics of the operations, the two cases seem oddly different: the 
group separator tests for ',' and '.' and ' ', but the decimal separator tests for 
'.' and ','. Why are they testing for different things, and in the opposite order?


I looked through the 810 locales available in Oracle JDK 15, and here's what I 
found:

Decimal Separators:

U+002c 359
U+002e 400
U+066b 51

Grouping Separators:

U+002c 378
U+002e 167
U+00a0 158
U+066c 51
U+2019 12
U+202f 44

I don't see a space used as a gropuing separator, but note that U+00a0 is no-break 
space. Does that occur often enough to special-case?


(Note that these numbers don't take into account how often each locale is used, nor 
are the 810 locales in the JDK the universe of all possible locales. However, it 
seems likely that any other locales would not appear frequently enough to bother 
with a special case.)


I also see that the group separator code tests for ',' first whereas the decimal 
separator code tests for '.' first. I suppose this might be a femto-optimization 
that slightly favors English-speaking locales, but it seems gratuitous to me. (If 
you can come up with a benchmark that illustrates the difference, be my guest!) :-)


I'm wondering if it might be nicer to extract this computation into a little utility 
method nearby:



// Escapes a separator character to prevent it from being
// interpreted as a regex metacharacter. > static String 
escapeSeparator(char ch) {
if (ch == ',' || ch == '.') {
return "\\" + ch;
} else {
return "\\x{" + Integer.toHexString(ch) + "}";
}
}


(Note that I rewrote the comment that already existed above the lines you changed, 
as it seemed misleading.)


The calling code would then be changed to:


groupSeparator = escapeSeparator(dfs.getGroupingSeparator());
decimalSeparator = escapeSeparator(dfs.getDecimalSeparator());


I haven't benchmarked this. However, this would be called from within useLocale(), 
which is a low-frequency operation compared to the cases you benchmarked.


Also, given that U+00a0 occurs relatively frequently among locales, it might be 
worthwhile also to special-case for it, but I don't have a strong opinion on that. 
This case would likely never occur for the decimal separator, but it doesn't bother 
me. I don't think it adds much value to have different special cases for the 
different separators.


**

Finally, can this be converted into a github PR? It'll have to be made into a PR 
sooner or later in order to be integrated into the JDK mainline. (Though I guess the 
update releases are still using the current email+webrev process for now, sorry.)


s'marks




On 9/21/20 9:48 PM, Martin Balao wrote:

Hi,

I'd like to propose a fix for JDK-8253451 [1] performance regression.

Webrev.00:

  * http://cr.openjdk.java.net/~mbalao/webrevs/8253451/8253451.webrev.00

As explained in [1], the idea for the fix is to optimize the regexp
string for the most common group and decimal separator characters across
locales. Please note that there are a few locales where the proposed
optimization does not apply and the slower -but safe- regexp introduced
by 8236201 is used.

Testing:

  * No regressions observed in jdk/java/util/Scanner (5/5 passed)

  * Verified performance improvement back to what it was before 8236201
(on my Americas locale).

Thanks,
Martin.-

--
[1] - https://bugs.openjdk.java.net/browse/JDK-8253451

Re: RFR: 8247402: rewrite the implementation requirements for Map::compute()

2020-10-13 Thread Stuart Marks

On Tue, 13 Oct 2020 13:35:37 GMT, Pavel Rappo  wrote:

>> The proposed implementation behaves differently from the existing 
>> implementation in the case of `null` value. That is,
>> when `map.containsKey(key) == true && map.get(key) == null`. The difference 
>> is that the proposed implementation will
>> remove the `(key, null)` mapping if `newValue == null`, whereas the existing 
>> implementation will not.
>
> Perhaps I should clarify my previous comment. When I said "implementation" 
> what I meant was that pseudo-code inside the
> `@implSpec` tag, not the actual body of `default V compute(K key, 
> BiFunction
> remappingFunction)`.

This change is to a normative portion of the specification, and as such will 
require a CSR.

-

PR: https://git.openjdk.java.net/jdk/pull/451

Re: RFR: 8254161: Prevent instantiation of EnumSet subclasses through deserialization

2020-10-14 Thread Stuart Marks

On Mon, 12 Oct 2020 13:47:46 GMT, Chris Hegarty  wrote:

> TL;DR add EnumSet::readObjectNoData()
> 
> EnumSet is an exemplar of the Serialization Proxy Pattern. As such, it
> should strictly implement that pattern and demonstrate how best to
> defend against inappropriate instantiation through deserialization.
> 
> EnumSet is an extensible class. There are two subclasses in the JDK,
> RegularEnumSet and JumboEnumSet. Since the serialization of an EnumSet
> object writes a replacement object to the serial stream, a serial proxy
> object, then stream objects of type RegularEnumSet or JumboEnumSet are
> not expected in the serial stream. However, if they are present in the
> serial stream, then, during deserialization, the EnumSet::readObject
> method will be invoked. EnumSet::readObject unconditionally throws an
> exception, thus preventing further deserialization of the stream object.
> In this way, stream objects that are subclasses of EnumSet are prevented
> from being instantiated through deserialization. But this is not
> sufficient to prevent such in all scenarios.
> 
> A stream object whose local class equivalent of the specified stream
> class descriptor is a subclasses of EnumSet, but whose specified stream
> class descriptor does not list EnumSet as a superClass, may be
> instantiated through deserialization. Since the stream class descriptor
> does not list EnumSet as a superclass, then the defensive
> EnumSet::readObject is never invoked. To prevent such objects from
> being deserialized, an EnumSet::readObjectNoData() should be added -
> whose implementation unconditionally throws an exception, similar to
> that of the existing EnumSet::readObject.

Marked as reviewed by smarks (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/611

RFR: 8255016: ConstantDescs.FALSE claims it represents TRUE

2020-10-19 Thread Stuart Marks

This is a fix for "just a typo" or copy/paste error, but it probably requires a 
CSR since it changes a normative
portion of the spec.

-

Commit messages:
 - 8255016: ConstantDescs.FALSE claims it represents TRUE

Changes: https://git.openjdk.java.net/jdk/pull/744/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=744&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8255016
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/744.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/744/head:pull/744

PR: https://git.openjdk.java.net/jdk/pull/744

Integrated: 8255016: ConstantDescs.FALSE claims it represents TRUE

2020-10-19 Thread Stuart Marks

On Mon, 19 Oct 2020 18:08:28 GMT, Stuart Marks  wrote:

> This is a fix for "just a typo" or copy/paste error, but it probably requires 
> a CSR since it changes a normative
> portion of the spec.

This pull request has now been integrated.

Changeset: bf19581a
Author:Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/bf19581a
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8255016: ConstantDescs.FALSE claims it represents TRUE

Reviewed-by: bpb, jvernee, mchung, rriggs

-

PR: https://git.openjdk.java.net/jdk/pull/744

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

2020-10-27 Thread Stuart Marks

On Sat, 24 Oct 2020 22:22:56 GMT, Peter Levart  wrote:

>> Reference instances should not be leaked and so I don't see very common that 
>> caller of `Reference::get` does not know the referent's type.   It also 
>> depends on the `refersTo` check against `null` vs an object.  Any known use 
>> case would be helpful if any (some existing code that wants to call 
>> `refersTo` to compare a `Reference` of raw type with an object of unknown 
>> type).
>> 
>> FWIW, when converting a few use of `Reference::get` to `refersTo` in JDK, 
>> there is only one case (`equals(Object o)` method that needs the cast.
>> 
>> http://cr.openjdk.java.net/~mchung/jdk15/webrevs/8188055/jdk-use-refersTo/index.html
>
> @mlchung I don't have many known use cases, but how about 
> WeakHashMap.containsKey(Object key) for example? Currently 
> `WeakHashMap.Entry extends  WeakReference` but it would be more 
> type safe if it extended `WeakReference`. In that case an 
> `entry.refersTo(key)` would not compile...
> What I'm trying to say is that even if `Reference` instances are not 
> "leaked", you can get an untyped object reference from outside and you may 
> want to identity-compare it with the Reference's referent.

Some thoughts regarding the parameter type of refersTo. Summary: I think 
`refersTo(T)` is fine and that we don't want to change it to `refersTo(Object)`.

I don't think we have a migration issue similar to generifying collections, 
where there was a possibility of changing `contains(Object)` to `contains(E)`. 
If that had been done, it would have been a source compatibility issue, because 
changing the signature of the method potentially affects existing code that 
calls the method. That doesn't apply here because we're adding a new method.

The question now falls to whether it's preferable to have more convenience with 
`refersTo(Object)` or more type-safety with `refersTo(T)`. With the generic 
collections issue, the migration issue probably drove the decision to keep 
`contains(Object)`, but this has resulted in a continual set of complaints 
about the lack of an error when code passes an instance of the "wrong" type. I 
think that kind of error is likely to occur with `refersTo`. Since we don't 
have a source compatibility issue here, we can choose the safer API and avoid 
this kind of problem entirely.

The safer API does raise the possibility of having to add inconvenient 
unchecked casts and local variables in certain places, but I think Mandy's 
comment about the code already having a reference of the "right" type is 
correct. Her prototype webrev linked above shows that having to add unchecked 
casts is fairly infrequent.

-

PR: https://git.openjdk.java.net/jdk/pull/498

Re: RFR: 8188055: (ref) Add Reference::refersTo predicate [v6]

2020-10-27 Thread Stuart Marks

On Wed, 21 Oct 2020 02:28:30 GMT, Kim Barrett  wrote:

>> Finally returning to this review that was started in April 2020.  I've
>> recast it as a github PR.  I think the security concern raised by Gil
>> has been adequately answered.
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-April/029203.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-July/030401.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-August/030677.html
>> https://mail.openjdk.java.net/pipermail/hotspot-gc-dev/2020-September/030793.html
>> 
>> Please review a new function: java.lang.ref.Reference.refersTo.
>> 
>> This function is needed to test the referent of a Reference object without
>> artificially extending the lifetime of the referent object, as may happen
>> when calling Reference.get.  Some garbage collectors require extending the
>> lifetime of a weak referent when accessed, in order to maintain collector
>> invariants.  Lifetime extension may occur with any collector when the
>> Reference is a SoftReference, as calling get indicates recent access.  This
>> new function also allows testing the referent of a PhantomReference, which
>> can't be accessed by calling get.
>> 
>> The new function uses native methods whose implementations are in the VM so
>> they can use the Access API.  It is the intent that these methods will be
>> intrinsified by optimizing compilers like C2 or graal, but that hasn't been
>> implemented yet.  Bear that in mind before rushing off to change existing
>> uses of Reference.get.
>> 
>> There are two native methods involved, one in Reference and an override in
>> PhantomReference, both package private in java.lang.ref. The reason for this
>> split is to simplify the intrinsification. This is a change from the version
>> from April 2020; that version had a single native method in Reference,
>> implemented using the ON_UNKNOWN_OOP_REF Access reference strength category.
>> However, adding support for that category in the compilers adds significant
>> implementation effort and complexity. Splitting avoids that complexity.
>> 
>> Testing:
>> mach5 tier1
>> Locally (linux-x64) verified the new test passes with various garbage 
>> collectors.
>
> Kim Barrett has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   improve wording in refersTo javadoc

Marked as reviewed by smarks (Reviewer).

-

PR: https://git.openjdk.java.net/jdk/pull/498

RFR: 8180352: Add Stream.toList() method

2020-11-02 Thread Stuart Marks

This change introduces a new terminal operation on Stream. This looks like a 
convenience method for Stream.collect(Collectors.toList()) or 
Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
method directly on Stream enables it to do what can't easily by done by a 
Collector. In particular, it allows the stream to deposit results directly into 
a destination array (even in parallel) and have this array be wrapped in an 
unmodifiable List without copying.

In the past we've kept most things from the Collections Framework as 
implementations of Collector, not directly on Stream, whereas only fundamental 
things (like toArray) appear directly on Stream. This is true of most 
Collections, but it does seem that List is special. It can be a thin wrapper 
around an array; it can handle generics better than arrays; and unlike an 
array, it can be made unmodifiable (shallowly immutable); and it can be 
value-based. See John Rose's comments in the bug report:

https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065

This operation is null-tolerant, which matches the rest of Streams. This isn't 
specified, though; a general statement about null handling in Streams is 
probably warranted at some point.

Finally, this method is indeed quite convenient (if the caller can deal with 
what this operation returns), as collecting into a List is the most common 
stream terminal operation.

-

Commit messages:
 - 8180352: Add Stream.toList() method

Changes: https://git.openjdk.java.net/jdk/pull/1026/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8180352
  Stats: 405 lines in 6 files changed: 358 ins; 23 del; 24 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1026.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-03 Thread Stuart Marks

On Tue, 3 Nov 2020 10:58:25 GMT, Stephen Colebourne  
wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 211:
> 
>> 209: }
>> 210: 
>> 211: switch (input.length) { // implicit null check of elements
> 
> Was a variable renamed? Should "elements" be "input"?

Yes, actually that comment belongs up above. I'll fix it, thanks.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-03 Thread Stuart Marks

On Tue, 3 Nov 2020 11:05:21 GMT, Stephen Colebourne  
wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> src/java.base/share/classes/java/util/stream/Stream.java line 1168:
> 
>> 1166:  * Accumulates the elements of this stream into a {@code List}. 
>> The elements in
>> 1167:  * the list will be in this stream's encounter order, if one 
>> exists. There are no
>> 1168:  * guarantees on the implementation type, mutability, 
>> serializability, or
> 
> It would be useful for callers to feel more confident that they will get an 
> immutable instance. In java.time.* we have wording like "This interface 
> places no restrictions on the mutability of implementations, however 
> immutability is strongly recommended." Could something like that work here, 
> emphasising that everyone implementing this method should seek to return an 
> immutable list?

Yes, good point, the "no guarantee of mutability" clashes with the later 
statement about the possibility of the returned instance being value-based, 
which strongly implies immutability. I'll work on tuning this up to be a 
stronger statement on immutability, while retaining "no-guarantee" for 
implementation type, serializability, etc. I think we do want to preserve 
future implementation flexibility in those areas.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-03 Thread Stuart Marks

On Tue, 3 Nov 2020 10:06:26 GMT, Daniel Fuchs  wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> Should there be a test that tests the default implementation in 
> `j.u.s.Stream`? Or maybe there is and I missed that?

@dfuch wrote:
> Should there be a test that tests the default implementation in 
> `j.u.s.Stream`? Or maybe there is and I missed that?

The test method `testDefaultOps` does that. The stream test framework has a 
thing called `DefaultMethodStreams` that delegates everything except default 
methods to another Stream instance, so this should test the default 
implementation.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-03 Thread Stuart Marks





On 11/3/20 3:10 AM, Florian Weimer wrote:

I'd expect a test here that if the list contains a null element, `List::copyOf` 
throws `NullPointerException`.


The new Stream.toList() actually permits null elements (by design) so it goes 
through a different path than List::copyOf. The new tests' data provider does 
include nulls in the input, and these should be accepted.


Rejection of nulls for List::copyOf is be handled by tests in

test/jdk/java/util/List/ListFactories.java

s'marks

Re: RFR: 8180352: Add Stream.toList() method

2020-11-03 Thread Stuart Marks

On Tue, 3 Nov 2020 18:53:23 GMT, Stuart Marks  wrote:

>> Should there be a test that tests the default implementation in 
>> `j.u.s.Stream`? Or maybe there is and I missed that?
>
> @dfuch wrote:
>> Should there be a test that tests the default implementation in 
>> `j.u.s.Stream`? Or maybe there is and I missed that?
> 
> The test method `testDefaultOps` does that. The stream test framework has a 
> thing called `DefaultMethodStreams` that delegates everything except default 
> methods to another Stream instance, so this should test the default 
> implementation.

OK, there are rather too many forking threads here for me to try to reply to 
any particular message, so I'll try to summarize things and say what I intend 
to do.

Null tolerance. While part of me wants to prohibit nulls, the fact is that 
Streams pass through nulls, and toList() would be much less useful if it were 
to reject nulls. The affinity here is closer to Stream.toArray(), which allows 
nulls, rather than Collectors.to[Unmodifiable]List.

Unmodifiability. Unlike with nulls, this is where we've been taking a strong 
stand recently, where new constructs are unmodifiable ("shallowly immutable"). 
Consider the Java 9 unmodifiable collections, records, primitive classes, etc. 
-- they're all unmodifiable. They're data-race-free and are resistant to a 
whole class of bugs that arise from mutability.

Unfortunately, the combination of the above means that the returned List is 
neither like an ArrayList nor like an unmodifiable list produced by List.of() 
or by Collectors.toUnmodifiableList(). [Heavy sigh.] While I've been somewhat 
reluctant to introduce this new thing, the alternatives of trying to reuse one 
of the existing things are worse.

John and Rémi pointed out that the way I implemented this, using a subclass, 
reintroduces the possibility of problems with megamorphic dispatch which we had 
so carefully avoided by reducing the number of implementation classes in this 
area. I agree this is a problem.

I also had an off-line discussion with John where we discussed the 
serialization format, which unfortunately is somewhat coupled with this issue. 
(Fortunately I think it can mostly be decoupled.) Given that we're introducing 
a new thing, which is an unmodifiable-list-that-allows-nulls, this needs to be 
manifested in the serialized form of these new objects. (This corresponds to 
the new tag value of IMM_LIST_NULLS in the CollSer class.) The alternative is 
to reuse the existing serialized form, IMM_LIST, for both of these cases, 
relaxing it to allow nulls. This would be a serialization immutability problem. 
Suppose I had an application that created a data structure that used lists from 
List.of(), and I had a global assertion over that structure that it contained 
no nulls. Further suppose that I serialized and deserizalized this structure. 
I'd want that assertion to be preserved after deserialization. If another app 
(or a future version of this app) created the structure using Stream.to
 List(), this would allow nulls to leak into that structure and violate that 
assertion. Therefore, the result of Stream.toList() should not be 
serialization-compatible with List.of() et. al. That's why there's the new 
IMM_LIST_NULLS tag in the serial format.

However, the new representation in the serial format does NOT necessarily 
require the implementation to be a different class. I'm going to investigate 
collapsing ListN and ListNNullsAllowed back into a single class, while 
preserving the separate serialized forms. This should mitigate the concerns 
about megamorphic dispatch.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-05 Thread Stuart Marks

On Wed, 4 Nov 2020 09:46:32 GMT, Zheka Kozlov 
 wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 240:
> 
>> 238: static  List listFromTrustedArrayNullsAllowed(Object... 
>> input) {
>> 239: if (input.length == 0) {
>> 240: return (List) EMPTY_LIST;
> 
> If we return a `ListN` here, does this mean that 
> `Stream.of().toList().contains(null)` will throw an NPE? But this is 
> incorrect because `toList()` returns a null-tolerant List.

Yes, good point, we might need to have a null-tolerant empty list.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-05 Thread Stuart Marks

On Wed, 4 Nov 2020 23:03:02 GMT, Paŭlo Ebermann 
 wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 199:
> 
>> 197:  * safely reused as the List's internal storage, avoiding a 
>> defensive copy. Declared
>> 198:  * with Object... instead of E... as the parameter type so that 
>> varargs calls don't
>> 199:  * accidentally create an array of type other than Object[].
> 
> Why would that be a problem? If the resulting list is immutable, then the 
> actual array type doesn't really matter, right?

It's an implementation invariant that the internal array be Object[]. Having it 
be something other than Object[] can lead to subtle bugs. See 
[JDK-6260652](https://bugs.openjdk.java.net/browse/JDK-6260652) for example.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-05 Thread Stuart Marks

On Thu, 5 Nov 2020 19:47:43 GMT, Paul Sandoz  wrote:

>> This change introduces a new terminal operation on Stream. This looks like a 
>> convenience method for Stream.collect(Collectors.toList()) or 
>> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
>> method directly on Stream enables it to do what can't easily by done by a 
>> Collector. In particular, it allows the stream to deposit results directly 
>> into a destination array (even in parallel) and have this array be wrapped 
>> in an unmodifiable List without copying.
>> 
>> In the past we've kept most things from the Collections Framework as 
>> implementations of Collector, not directly on Stream, whereas only 
>> fundamental things (like toArray) appear directly on Stream. This is true of 
>> most Collections, but it does seem that List is special. It can be a thin 
>> wrapper around an array; it can handle generics better than arrays; and 
>> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
>> can be value-based. See John Rose's comments in the bug report:
>> 
>> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
>> 
>> This operation is null-tolerant, which matches the rest of Streams. This 
>> isn't specified, though; a general statement about null handling in Streams 
>> is probably warranted at some point.
>> 
>> Finally, this method is indeed quite convenient (if the caller can deal with 
>> what this operation returns), as collecting into a List is the most common 
>> stream terminal operation.
>
> test/jdk/java/util/stream/test/org/openjdk/tests/java/util/stream/ToListOpTest.java
>  line 73:
> 
>> 71: }
>> 72: 
>> 73: @Test(dataProvider = "withNull:StreamTestData", 
>> dataProviderClass = StreamTestDataProvider.class)
> 
> Given the non-default `toList()` implementation defers to the `toArray()` 
> terminal op there is no need for this and the following tests, which are 
> really designed to shake out the optimizations when producing and operating 
> on arrays.

OK, I'll remove all the tests starting from here to the end of the file. I'm 
assuming that's the set of tests you're referring to.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-05 Thread Stuart Marks

On Wed, 4 Nov 2020 23:18:29 GMT, Paŭlo Ebermann 
 wrote:

>> Changes requested by orio...@github.com (no known OpenJDK username).
>
> Looking at the linked issue, I see [this comment from Rémi 
> Forax](https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14171626&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14171626):
> 
>> Please talk with Brian about this change because it nulls a property of the 
>> Stream API we (the lambda-util group) have take time to keep.
> The whole Stream API doesn't depends on the Collection API, [...] so the 
> Stream API can be easily integrated with other collection API if in the 
> future we want by example add a persistent collection API with no link with 
> java.util.List.
> 
> That's an argument I can understand – as is, the Stream API works just as 
> well with collections from e.g. Scala's or Kotlin's (or Ceylon's) collection 
> libraries instead of the java.util ones, just using different collectors. 
> Adding a method which directly returns a java.util.List somewhat couples it 
> to the Java Collection API.
> 
> Now this was mentioned two and a half year ago. Did something change which 
> made this consideration irrelevant? I would expect at least some mention of 
> it in the discussion here.

@ePaul wrote:

> The Stream API works just as well with [third party] collection libraries 
> instead of the java.util ones, just using different collectors. Adding a 
> method which directly returns a java.util.List somewhat couples it to the 
> Java Collection API.
> 
> Now this was mentioned two and a half year ago. Did something change which 
> made this consideration irrelevant? I would expect at least some mention of 
> it in the discussion here.

The separation between streams and the java.util Collections Framework is a 
good design principle, but it isn't an ironclad rule. It's still easy to have 
streams create instances of other collections libraries using the Collector 
interface. What's different here is that the Collections Framework has "leaked 
into" streams a little bit more, so that they're now more interdependent. This 
doesn't seem to have any disadvantages; it seems unlikely that the Collections 
Framework will ever be unplugged from the JDK. However, the benefits are that a 
List is the closest thing we have to an unmodifiable array that also plays well 
with generics and that can be value-based; these benefits are considerable.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method

2020-11-05 Thread Stuart Marks

On Fri, 6 Nov 2020 03:01:42 GMT, Stuart Marks  wrote:

>> Looking at the linked issue, I see [this comment from Rémi 
>> Forax](https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14171626&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14171626):
>> 
>>> Please talk with Brian about this change because it nulls a property of the 
>>> Stream API we (the lambda-util group) have take time to keep.
>> The whole Stream API doesn't depends on the Collection API, [...] so the 
>> Stream API can be easily integrated with other collection API if in the 
>> future we want by example add a persistent collection API with no link with 
>> java.util.List.
>> 
>> That's an argument I can understand – as is, the Stream API works just as 
>> well with collections from e.g. Scala's or Kotlin's (or Ceylon's) collection 
>> libraries instead of the java.util ones, just using different collectors. 
>> Adding a method which directly returns a java.util.List somewhat couples it 
>> to the Java Collection API.
>> 
>> Now this was mentioned two and a half year ago. Did something change which 
>> made this consideration irrelevant? I would expect at least some mention of 
>> it in the discussion here.
>
> @ePaul wrote:
> 
>> The Stream API works just as well with [third party] collection libraries 
>> instead of the java.util ones, just using different collectors. Adding a 
>> method which directly returns a java.util.List somewhat couples it to the 
>> Java Collection API.
>> 
>> Now this was mentioned two and a half year ago. Did something change which 
>> made this consideration irrelevant? I would expect at least some mention of 
>> it in the discussion here.
> 
> The separation between streams and the java.util Collections Framework is a 
> good design principle, but it isn't an ironclad rule. It's still easy to have 
> streams create instances of other collections libraries using the Collector 
> interface. What's different here is that the Collections Framework has 
> "leaked into" streams a little bit more, so that they're now more 
> interdependent. This doesn't seem to have any disadvantages; it seems 
> unlikely that the Collections Framework will ever be unplugged from the JDK. 
> However, the benefits are that a List is the closest thing we have to an 
> unmodifiable array that also plays well with generics and that can be 
> value-based; these benefits are considerable.

> Simon Roberts wrote:

> This discussion of unmodifiable lists brings me back to the thought that
> there would be good client-side reasons for inserting an UnmodifiableList
> interface as a parent of LIst, not least because all our unmodifiable
> variants from the Collections.unmodifiableList proxy onward fail the Liskov
> substitution test for actually "being contract-fulfilling Lists".

At some point there probably will need to be a long article explaining all the 
issues here, but at the moment the best writeup I have is this one:

https://stackoverflow.com/a/57926310/1441122

TL;DR there are a few different ways to approach retrofitting something like 
this, but they all have enough compromises that the net benefits are unclear.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method [v2]

2020-11-06 Thread Stuart Marks

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Merge ListNNullsAllowed into ListN. Update spec for Stream.toList.
  Add nulls-allowed empty list. Simplify indexOf/lastIndexOf.
  Reduce tests, add contains(null) tests.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1026/files
  - new: https://git.openjdk.java.net/jdk/pull/1026/files/3e05564d..cf849755

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=00-01

  Stats: 181 lines in 3 files changed: 16 ins; 117 del; 48 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1026.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8180352: Add Stream.toList() method [v2]

2020-11-09 Thread Stuart Marks

On Sun, 8 Nov 2020 15:55:58 GMT, Peter Levart  wrote:

>> Hi Stuart,
>> 
>> I would like to discuss the serialization. You introduce new 
>> CollSer.IMM_LIST_NULLS type of immutable collection. This means that if this 
>> change goes into JDK16 for example, JDK15 and before will not be able to 
>> deserialize such list as they know nothing about IMM_LIST_NULLS even if such 
>> lists don't contain nulls. The reason you say to chose new type of 
>> serialization format is the following:
>> 
>>> "Suppose I had an application that created a data structure that used lists 
>>> from List.of(), and I had a global assertion over that structure that it 
>>> contained no nulls. Further suppose that I serialized and deserizalized 
>>> this structure. I'd want that assertion to be preserved after 
>>> deserialization. If another app (or a future version of this app) created 
>>> the structure using Stream.to
>>>  List(), this would allow nulls to leak into that structure and violate 
>>> that assertion. Therefore, the result of Stream.toList() should not be 
>>> serialization-compatible with List.of() et. al. That's why there's the new 
>>> IMM_LIST_NULLS tag in the serial format"
>> 
>> I don't quite get this reasoning. Let's try to decompose the reasoning 
>> giving an example. Suppose we had the following data structure:
>> 
>> public class Names implements Serializable {
>>   private final List names;
>>   Names(List names) {
>> this.names = names;
>>   }
>>   public List names() { return names; }
>> }
>> 
>> App v1 creates such structures using new Names(List.of(...)) and 
>> serializes/deserializes them. They keep the invariant that no nulls are 
>> present. Now comes App v2 that starts using new Names(stream.toList()) which 
>> allows nulls to be present. When such Names instance from app v2 is 
>> serialized and then deserialized in app v1, nulls "leak" into data structure 
>> of app v1 that does not expect them.
>> 
>> But the question is how does having a separate CollSer.IMM_LIST_NULLS type 
>> prevent that from happening?
>
> I can see that having a separate IMM_LIST_NULLS type might be necessary to 
> preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf 
> methods...
> 
> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from 
> AbstractImmutableList:
> 
> @Override
> public boolean equals(Object o) {
> if (o == this) {
> return true;
> }
> 
> if (!(o instanceof List)) {
> return false;
> }
> 
> Iterator oit = ((List) o).iterator();
> for (int i = 0, s = size(); i < s; i++) {
> if (!oit.hasNext() || !get(i).equals(oit.next())) {
> return false;
> }
> }
> return !oit.hasNext();
> }
> and
> public int hashCode() {
> int hash = 1;
> for (int i = 0, s = size(); i < s; i++) {
> hash = 31 * hash + get(i).hashCode();
> }
> return hash;
> }
> 
> ...which means they will throw NPE when the list contains null. The same goes 
> for SubList.

@plevart wrote:
> But the question is how does having a separate CollSer.IMM_LIST_NULLS type 
> prevent that from happening?

When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK, 
it'll throw InvalidObjectException since that tag isn't valid on older JDKs. 
Obviously this is still an error, but it's a fail-fast approach that avoids 
letting nulls leak into a data structure where they might cause a problem some 
arbitrary time later.

> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from 
> AbstractImmutableList [...] which means they will throw NPE when the list 
> contains null. The same goes for SubList.

Good catch! Yes, this is a problem. I'll do some rearranging here and add more 
test cases. Thanks for spotting this.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

RFR: 8256152: tests fail because of ambiguous method resolution

2020-11-17 Thread Stuart Marks

Added a cast in the right place, thanks to @jonathan-gibbons.

-

Commit messages:
 - 8256152: tests fail because of ambiguous method resolution

Changes: https://git.openjdk.java.net/jdk/pull/1274/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1274&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8256152
  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1274/head:pull/1274

PR: https://git.openjdk.java.net/jdk/pull/1274

Re: RFR: 8180352: Add Stream.toList() method [v2]

2020-11-17 Thread Stuart Marks

On Tue, 10 Nov 2020 09:34:56 GMT, Peter Levart  wrote:

>> I can see that having a separate IMM_LIST_NULLS type might be necessary to 
>> preserve the allows-null/disallows-null behaviour of indexOf and lastIndexOf 
>> methods...
>> 
>> NOTE ALSO that ListN.equals(o) and ListN.hashCode() are inherited from 
>> AbstractImmutableList:
>> 
>> @Override
>> public boolean equals(Object o) {
>> if (o == this) {
>> return true;
>> }
>> 
>> if (!(o instanceof List)) {
>> return false;
>> }
>> 
>> Iterator oit = ((List) o).iterator();
>> for (int i = 0, s = size(); i < s; i++) {
>> if (!oit.hasNext() || !get(i).equals(oit.next())) {
>> return false;
>> }
>> }
>> return !oit.hasNext();
>> }
>> and
>> public int hashCode() {
>> int hash = 1;
>> for (int i = 0, s = size(); i < s; i++) {
>> hash = 31 * hash + get(i).hashCode();
>> }
>> return hash;
>> }
>> 
>> ...which means they will throw NPE when the list contains null. The same 
>> goes for SubList.
>
>> @plevart wrote:
>> 
>> > But the question is how does having a separate CollSer.IMM_LIST_NULLS type 
>> > prevent that from happening?
>> 
>> When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK, 
>> it'll throw InvalidObjectException since that tag isn't valid on older JDKs. 
>> Obviously this is still an error, but it's a fail-fast approach that avoids 
>> letting nulls leak into a data structure where they might cause a problem 
>> some arbitrary time later.
>> 
> 
> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both 
> apps run on JDK16+, there will be no exception.
> 
> What I'm trying to say is that the same problem of injecting unexpected nulls 
> via serialization/deserialization can happen also if App V2 starts using 
> ArrayList to construct the data structure and serialize it while App V1 
> deserializes it and expects non-null values only. App V1 would already have 
> to guard against null values during deserialization in that case, because 
> possibility of null values in deserialized data structure is nothing new for 
> App V1.

@plevart wrote:
> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both 
> apps run on JDK16+, there will be no exception.

Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility across 
JDK releases. There are lots of ways an app can make incompatible changes to 
the serialized forms of its objects that we have no way of detecting.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8256152: tests fail because of ambiguous method resolution [v2]

2020-11-17 Thread Stuart Marks

> Added a cast in the right place, thanks to @jonathan-gibbons.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  cast to double instead of Object

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1274/files
  - new: https://git.openjdk.java.net/jdk/pull/1274/files/4a401ed2..6875beb1

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1274&range=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1274&range=00-01

  Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1274.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1274/head:pull/1274

PR: https://git.openjdk.java.net/jdk/pull/1274

Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v9]

2020-11-17 Thread Stuart Marks

On Tue, 17 Nov 2020 21:21:47 GMT, Roger Riggs  wrote:

>> Ian Graves has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   Adding test coverage. Tweaking wording in docs.
>
> test/jdk/java/util/IllegalFormatException/ArgumentIndexException.java line 1:
> 
>> 1: /*
> 
> Typically, using the TestNG framework is preferable for new tests.
> In addition to a convenient set of Asserts that check and print expected vs 
> actual and message
> it includes patterns for testing for expected exceptions.

Yes, these tests are amenable to TestNG's `assertThrows` method. That might be 
all you need; we generally aren't too concerned about the exact content and 
formatting of the exception's message. However, if you want to make additional 
assertions over the thrown exception, it can be obtained using `expectThrows` 
instead of `assertThrows`.

-

PR: https://git.openjdk.java.net/jdk/pull/516

Re: RFR: 8180352: Add Stream.toList() method [v2]

2020-11-18 Thread Stuart Marks

On Tue, 17 Nov 2020 20:04:58 GMT, Stuart Marks  wrote:

>>> @plevart wrote:
>>> 
>>> > But the question is how does having a separate CollSer.IMM_LIST_NULLS 
>>> > type prevent that from happening?
>>> 
>>> When a serialized list with IMM_LIST_NULLS is deserialized on an older JDK, 
>>> it'll throw InvalidObjectException since that tag isn't valid on older 
>>> JDKs. Obviously this is still an error, but it's a fail-fast approach that 
>>> avoids letting nulls leak into a data structure where they might cause a 
>>> problem some arbitrary time later.
>>> 
>> 
>> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both 
>> apps run on JDK16+, there will be no exception.
>> 
>> What I'm trying to say is that the same problem of injecting unexpected 
>> nulls via serialization/deserialization can happen also if App V2 starts 
>> using ArrayList to construct the data structure and serialize it while App 
>> V1 deserializes it and expects non-null values only. App V1 would already 
>> have to guard against null values during deserialization in that case, 
>> because possibility of null values in deserialized data structure is nothing 
>> new for App V1.
>
> @plevart wrote:
>> Yes, but that is JDK16+ vs. JDK15- and not App V1 vs. App V2 thing. If both 
>> apps run on JDK16+, there will be no exception.
> 
> Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility 
> across JDK releases. There are lots of ways an app can make incompatible 
> changes to the serialized forms of its objects that we have no way of 
> detecting.

>> Sure, the IMM_LIST_NULLS tag only helps with serialization compatibility 
>> across JDK releases.

> I would say it goes the other way - it worsens the serialization
compatibility.

OK, I was imprecise. The IMM_LIST_NULLS tag has an effect only on serialization 
across JDK releases, not changes to the application's serialization format 
using the same JDK release, or even on many changes to the app's serialization 
format across JDK releases. By "helps with serialization compatibility" I meant 
that this new serialized form helps the general issue of serialization 
compatibility (really, incompatibility) by failing fast in certain cases, 
instead of possibly allowing polluted data to leak into the receiving 
application and causing some arbitrary exception later during the run.

But as you noted last, this is a different kind of object, and it has different 
behavior, so it needs a different encoding in the serialized form.

I'll update this PR shortly with changes to fix null handling and other issues.

-

PR: https://git.openjdk.java.net/jdk/pull/1026

Integrated: 8256152: tests fail because of ambiguous method resolution

2020-11-18 Thread Stuart Marks

On Tue, 17 Nov 2020 20:01:37 GMT, Stuart Marks  wrote:

> Added a cast in the right place, thanks to @jonathan-gibbons.

This pull request has now been integrated.

Changeset: 646c2002
Author:    Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/646c2002
Stats: 1 line in 1 file changed: 0 ins; 0 del; 1 mod

8256152: tests fail because of ambiguous method resolution

Reviewed-by: psandoz

-

PR: https://git.openjdk.java.net/jdk/pull/1274

Re: RFR: 8180352: Add Stream.toList() method [v3]

2020-11-18 Thread Stuart Marks

> This change introduces a new terminal operation on Stream. This looks like a 
> convenience method for Stream.collect(Collectors.toList()) or 
> Stream.collect(Collectors.toUnmodifiableList()), but it's not. Having this 
> method directly on Stream enables it to do what can't easily by done by a 
> Collector. In particular, it allows the stream to deposit results directly 
> into a destination array (even in parallel) and have this array be wrapped in 
> an unmodifiable List without copying.
> 
> In the past we've kept most things from the Collections Framework as 
> implementations of Collector, not directly on Stream, whereas only 
> fundamental things (like toArray) appear directly on Stream. This is true of 
> most Collections, but it does seem that List is special. It can be a thin 
> wrapper around an array; it can handle generics better than arrays; and 
> unlike an array, it can be made unmodifiable (shallowly immutable); and it 
> can be value-based. See John Rose's comments in the bug report:
> 
> https://bugs.openjdk.java.net/browse/JDK-8180352?focusedCommentId=14133065&page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel#comment-14133065
> 
> This operation is null-tolerant, which matches the rest of Streams. This 
> isn't specified, though; a general statement about null handling in Streams 
> is probably warranted at some point.
> 
> Finally, this method is indeed quite convenient (if the caller can deal with 
> what this operation returns), as collecting into a List is the most common 
> stream terminal operation.

Stuart Marks has updated the pull request incrementally with one additional 
commit since the last revision:

  Adjust List.copyOf to null-check and copy allowNulls lists.
  Fix equals, hashCode, indexOf, lastIndexOf to handle nulls properly.
  Add MOAT tests for new lists; add equals and hashCode tests.

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1026/files
  - new: https://git.openjdk.java.net/jdk/pull/1026/files/cf849755..15beacd2

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk&pr=1026&range=01-02

  Stats: 136 lines in 3 files changed: 104 ins; 22 del; 10 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1026.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1026/head:pull/1026

PR: https://git.openjdk.java.net/jdk/pull/1026

Re: RFR: 8253459: Formatter treats index, width and precision > Integer.MAX_VALUE incorrectly [v12]

2020-11-18 Thread Stuart Marks

On Wed, 18 Nov 2020 22:57:19 GMT, Ian Graves  wrote:

>> The `java.util.Formatter` format specifies support for field widths, 
>> argument indexes, or precision lengths of a field that relate to the 
>> variadic arguments supplied to the formatter. These numbers are specified by 
>> integers, sometimes negative. For argument index, it's specified in the 
>> documentation that the highest allowed argument is limited by the largest 
>> possible index of an array (ie the largest possible variadic index), but for 
>> the other two it's not defined. Moreover, what happens when a number field 
>> in a string is too large or too small to be represented by a 32-bit integer 
>> type is not defined.
>> 
>> This fix adds documentation to specify what error behavior occurs during 
>> these cases. Additionally it adds an additional exception type to throw when 
>> an invalid argument index is observed.
>> 
>> A CSR will be required for this PR.
>
> Ian Graves has updated the pull request incrementally with one additional 
> commit since the last revision:
> 
>   'unrepresentable' to 'not representable'

Marked as reviewed by smarks (Reviewer).

test/jdk/java/util/IllegalFormatException/TestFormatSpecifierBounds.java line 
48:

> 46: String r = String.format("%2147483648$s", "A", "B");
> 47: });
> 48: //assertEquals(e.getMessage(), "Illegal format argument index = " 
> + Integer.MIN_VALUE);

Extraneous comment?

-

PR: https://git.openjdk.java.net/jdk/pull/516

RFR: 8037384: Fix wording in Javadoc of java.io.Serializable

2020-11-18 Thread Stuart Marks

8231547: Serializable class doc should link to serialization specification

Rewrite a couple confusing sentences in the Serializable class doc. This does 
affect normative text, but the edits are primarily to focus and clarify the 
text, not to make any semantic changes. Thus, a CSR request shouldn't be 
required for this change.

Also add and adjust some links and link markup to the Java Object Serialization 
Specification.

-

Commit messages:
 - 8037384: Fix wording in Javadoc of java.io.Serializable

Changes: https://git.openjdk.java.net/jdk/pull/1306/files
 Webrev: https://webrevs.openjdk.java.net/?repo=jdk&pr=1306&range=00
  Issue: https://bugs.openjdk.java.net/browse/JDK-8037384
  Stats: 27 lines in 5 files changed: 5 ins; 1 del; 21 mod
  Patch: https://git.openjdk.java.net/jdk/pull/1306.diff
  Fetch: git fetch https://git.openjdk.java.net/jdk pull/1306/head:pull/1306

PR: https://git.openjdk.java.net/jdk/pull/1306

RFR (s): 8217412 deprecate rmic for removal

2019-05-30 Thread Stuart Marks


Hi all,

Please review this patch and CSR request for upgrading the deprecation status of 
the rmic to from ordinary to terminal (i.e., conceptually set forRemoval=true, 
though there are no actual annotations involved here).


There are no code changes, just documentation changes and changes to the warning 
message that rmic emits when run.


Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8217412/webrev.0/

(Sorry, the long lines in these files make most of the webrev output
unreadable. It might be easiest to look directly at the patch.)

JIRA bug:

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

Also please review this CSR request:

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

If you review the CSR, please edit its JIRA issue and add yourself to the 
"Reviewed By" field.


Thanks,

s'marks

RFR(xs): 8225315 test java/util/ArrayDeque/WhiteBox.java isn't part of the jdk_collections test group

2019-06-04 Thread Stuart Marks


Hi all,

This test was added some time ago, but it wasn't added to the jdk_collections 
test group. Please review the patch appended below, which adds it to the right 
group. (The test was still run as part of the catch-all jdk_util_other group 
though.)


Bug link: https://bugs.openjdk.java.net/browse/JDK-8225315

s'marks


diff -r ef23ea332077 test/jdk/TEST.groups
--- a/test/jdk/TEST.groups  Tue Jun 04 11:55:51 2019 -0700
+++ b/test/jdk/TEST.groups  Tue Jun 04 17:52:11 2019 -0700
@@ -137,6 +137,7 @@
 java/util/AbstractList \
 java/util/AbstractMap \
 java/util/AbstractSequentialList \
+java/util/ArrayDeque \
 java/util/ArrayList \
 java/util/Arrays \
 java/util/BitSet \

Re: RFR(xs): 8225315 test java/util/ArrayDeque/WhiteBox.java isn't part of the jdk_collections test group

2019-06-04 Thread Stuart Marks


On 6/4/19 5:56 PM, Joseph D. Darcy wrote:

Should the test be removed from the catch-all group as part of this change?


Ah, good question. But the answer is that it doesn't need to be. The "catch-all" 
group jdk_util_other is defined thus:


--

# All util components not part of some other util category
jdk_util_other = \
java/util \
sun/util \
-:jdk_collections \
-:jdk_concurrent \
-:jdk_stream

--

This syntax defines jdk_util_other to be the tests in the java/util and sun/util 
directories, MINUS the tests in the jdk_collections and a couple other groups. 
Thus adding the test to jdk_collections automatically removes it from 
jdk_util_other.


s'marks

RFR(s): 8205131: remove Runtime trace methods

2019-06-04 Thread Stuart Marks


Hi all,

Please review this changeset and CSR request that remove the traceInstructions() 
and traceMethodCalls() methods from java.lang.Runtime. These methods have been 
deprecated for removal since Java 9, and they do nothing. I've also removed a 
couple mentions of these methods from some tests.


After this changeset, the only times these methods are mentioned is in javac's 
symbol tables (for example, make/data/symbols/java.base-9.sym.txt) where they 
are kept because they are present in earlier releases.


They are also mentioned in the file

test/hotspot/jtreg/runtime/appcds/ExtraSymbols.symbols.txt

However, this file has a comment


68 -1: # The values in this file are only used for testing the operation of
63 -1: # adding extra symbols into the CDS archive. None of the values
70 -1: # are interpreted in any way. So even if they contain names of classes
70 -1: # that have been renamed or removed, or string literals that have been
66 -1: # changed or remove from Java source code, it would not affect the
26 -1: # correctness of the test.


so it seems that leaving mention of these methods in this file is harmless. 
Based on this comment I've decided not to change this file. Nonetheless, I'm 
including hotspot-dev in this review to make sure this is ok. (I seem to recall 
a similar issue came up the last time I removed something.)


Bug:

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

Webrev:

http://cr.openjdk.java.net/~smarks/reviews/8205131/webrev.0/

CSR request:

https://bugs.openjdk.java.net/browse/JDK-8225330
(if you review, please edit this issue and add yourself to the
Reviewed By field)

Thanks,

s'marks

Re: RFR JDK-8225339 Optimize HashMap.keySet()/HashMap.values()/HashSet toArray() methods

2019-06-05 Thread Stuart Marks





On 6/5/19 7:27 AM, Claes Redestad wrote:

On 2019-06-05 15:49, Tagir Valeev wrote:
In particular it's never mentioned in HashSet, HashMap or Collection spec that 
toArray implements a fail-fast behavior: this is said only about the 
iterator() method. 


that's not entirely true, since the @implSpec for toArray on
AbstractCollection states it is equivalent to iterating over the
collection[1].

So since the implementations you're changing inherit their current
implementation from AbstractCollection and the iterators of HashMap is
specified to be fail-fast, then that behavior can be argued to be
specified also for the affected toArray methods.

I'm not fundamentally objecting to the behavior change, but I do think
it needs careful review and a CSR (or at least plenty of reviewers
agreeing that one isn't needed).


OK, let's slow down here a bit.

The fail-fast, CME-throwing behavior is primarily of interest when iteration is 
external, that is, it's driven by the application making a succession of calls 
on the Iterator. The main concern arises when the application makes other 
modifications to the collection being iterated, between calls to the Iterator.


There is always the possibility of modification by some other thread, and this 
is mentioned obliquely in the spec near "fail-fast", but given memory visibility 
issues it's pretty much impossible to make any concrete statements about what 
happens if modifications are made by another thread.


In the case of these methods, the iteration occurs entirely within the context 
of a single method call. There's no possibility of the application making a 
concurrent modification on the same thread. So, while the fail-fast behavior is 
being changed, strictly speaking, I consider it "de minimis", as it's pretty 
difficult for an application to observe this change in behavior.


Regarding the @implSpec, that applies only to the implementation in 
AbstractCollection, and @implSpec is not inherited. Again, it is a change in 
behavior since this method is being changed from being inherited to being 
overridden, but it's not a specification issue.


In any case I don't think the concurrent modification behavior change is an 
issue to be concerned about.


**

Now, regarding visible changes in behavior, it's quite easy to observe this 
change in behavior, at least from a subclass of HashSet. A HashSet subclass 
could override the iterator() method and detect that it was called when 
toArray() is called. With this change, toArray() is overridden, and so 
iterator() would no longer be called in this circumstance.


This kind of change is generally allowable, but we have had complaints about the 
pattern of self-calls changing from release to release. This isn't a reason NOT 
to make the change, but the fact is that it does make changes to the visible 
behavior, and this potentially does affect actual code. (Code that I'd consider 
poorly written, but still.)


I talked this over with Joe Darcy (CSR chair) and we felt that it would be 
prudent to file a CSR a request to document the behavior change.


**

Some comments on the code.

Overall I think the changes are going in the right direction. It's amazing that 
after all this time, there are still cases in core collections that are 
inheriting the slow Abstract* implementations.


In HashMap, I think the factoring between keysToArray() and prepareArray() can 
be improved. The prepareArray() method itself is sensible, in that it implements 
to weird toArray(T[]) semantics of allocating a new array if the given one is 
too short, and it puts a 'null' at the right place if the given array is too 
long, and returns the possibly reallocated array.


The first thing that keysToArray() does is to call prepareArray() and use its 
return value.  What concerns me is that the no-arg toArray() method does this:


return keysToArray(new Object[size]);

so the array is created with exactly the right size; yet the first thing 
keysToArray() does is to call prepareArray(), which checks the size again and 
determines that nothing need be done.


It would be a small optimization to shift things around so that keysToArray() 
takes an array known to be "prepared" already and to remove its call to 
prepareArray(). Then, require the caller to call prepareArray() if necessary. 
Then we'd have this:


 953  public Object[] toArray() { return keysToArray(new Object[size]); }
 954  public  T[] toArray(T[] a) { return keysToArray(prepareArray(a)); }

I'm not terribly concerned about avoiding an extra method call. But other code 
in this file, for example like the following:


 930  Node[] tab;
 931  int idx = 0;
 932  if (size > 0 && (tab = table) != null) {

is written with an embedded assignment to 'tab', probably in order to avoid a 
useless load of the 'table' field if the size is zero. (This style occurs in 
several places.) So, if this code is making this level of micro-optimizations 
already, let's not waste it by calling extra meth

Re: RFR JDK-8225339 Optimize HashMap.keySet()/HashMap.values()/HashSet toArray() methods

2019-06-06 Thread Stuart Marks




On 6/5/19 9:25 PM, Tagir Valeev wrote:
Thanks for review. Indeed, you have a point about behavioral change. I filed a 
CSR:

https://bugs.openjdk.java.net/browse/JDK-8225393
Great, I've made some edits to the CSR and I've added myself as a reviewer. It 
should be ready for you to move into the Finalized state. Please wait for the 
CSR group (probably Joe Darcy) to mark it Approved before pushing this changeset.
I wanted to generate a specdiff and attach to CSR, but it seems that I don't 
know how to do it :(

I found nothing in OpenJDK Wiki about this. Could somebody help me?


As you noted elsewhere, the way the build runs javadoc shouldn't generate any 
specification differences, so we don't need to worry about specdiff. I've 
applied your patch to my personal tree and I've verified that there aren't any 
specification differences. (Indeed, the only difference is that the HTML files 
have an additional meta tag; I'm not sure of the significance of this, but it 
doesn't seem to impact the specification as far as I can see.)


Code-related suggestions are addressed in new webrev (along with Roger's 
suggestions):

http://cr.openjdk.java.net/~tvaleev/webrev/8225339/r2/


Small item: HashMap.values() and LinkedHashMap.values() still have the issue 
where the no-arg toArray() allocates a right-sized array and then calls 
toArray(T[]), which then calls prepareArray(), which rechecks the size. Thanks 
for changing the other places, but I think these should be changed too. 
Unfortunately I think this means introducing a helper method for values, similar 
to keysToArray(), and then both toArray() overloads can call it. But I think 
this makes good sense and aligns the implementation with the other toArray() 
implementations.


Oh, one more thing in LinkedHashMap.java:

565 public Object[] toArray() { return keysToArray(new Object[size]); }

Please add line breaks.

Thanks,

s'marks

Re: Suggestion for a more sensible implementation of EMPTY_MAP

2019-06-13 Thread Stuart Marks





On 6/10/19 7:34 AM, Abraham Marín Pérez wrote:

I agree that it makes sense for EMPTY_MAP to have the same logic as Map.of() or 
unmodifiable(new HashMap()), which means that my suggestion cannot be applied 
to just EMPTY_MAP... but maybe that’s ok: maybe we can change all of them? If 
we keep the default implementation for Map.computeIfPresent in EMPTY_MAP, 
Map.of(), unmodifiableMap, etc., instead of overriding it to throw an 
exception, then:

- For cases where the key isn’t present (regardless of empty or non-empt map), 
the call will be a safe no-op.
- For cases where the key is present, the call will still throw UOE via the 
implementation of put()

I believe this would still respect the spec for Map.computeIfPresent while 
providing a more forgiving implementation (in the sense that it will avoid 
throwing an exception when possible).


Hi Abraham,

The specification does not mandate one behavior or another. Either behavior is 
permitted, and in fact both behaviors are present in the JDK.


The second and more significant point is raised by your last statement 
suggesting a "more forgiving implementation." The question is, do we actually 
want a more forgiving implementation?


The collections specs define certain methods as "optional", but it's not clear 
whether this means that calling such a method should always throw an exception 
("strict" behavior) or whether the method should throw an exception only when it 
attempts to do something that's disallowed ("lenient" behavior).


Take for example the putAll() method. What happens if we do this?

Map map1 = Collections.emptyMap();
map1.putAll(otherMap);

The answer is that it depends on the state of otherMap. If otherMap is empty, 
then the operation succeeds (and does nothing). However, if otherMap is 
non-empty, then the operation will throw UnsupportedOperationException. That's 
an example of lenient behavior. What's notable about this is that you can't 
predict the outcome unless you know the state of otherMap.


Now, how about this example?

Map map2 = Map.of();
map2.putAll(otherMap);

This always throws UnsupportedOperationException, regardless of the state of 
otherMap. I'd call this strict behavior.


More recent implementations, including the Java 8 default methods, and the Java 
9 unmodifiable collections (Map.of et al), have all preferred strict behavior. 
This is because less information about the state of the system is necessary in 
order to reason about the code, which leads to fewer bugs, or at least earlier 
detection of bugs. It's unfortunate that emptyMap().putAll() has this lenient 
behavior, but we're stuck with it for compatibility reasons.


Now there is a slight wrinkle with computeIfPresent(). If we have

Map map = Collections.emptyMap();
map.computeIfPresent(key, remappingFunction);

then as you point out, this can never modify the map, since the key is never 
present. Thus, there isn't any behavior that's dependent upon additional state.


But, as Michael observed, there is now an inconsistency with Map.of() and 
Collections.unmodifiableMap(). So, continuing with your suggestion, we might 
change those implementations to allow computeIfPresent() as well. Thus,


Map map1 = Collections.emptyMap();
Map map2 = Map.of();
Map map3 = Collections.unmodifiableMap(new HashMap<>());

and then

{map1,map2,map3}.computeIfPresent(key, remappingFunction);

all are no-ops. Well, maybe. What if we had retained a reference to the HashMap 
wrapped by the unmodifiableMap, and then we added an element? Now it contains a 
mapping; what should happen then? Should computeIfPresent() throw an exception 
because it *might* attempt to modify the map? Or should it throw an exception 
*only* if it attempts to modify the map? State-dependent behavior has slipped 
back in.


We also need to consider the impact on the other compute* methods. Consider:

Map map = Collections.emptyMap();
map.compute(key, (k, v) -> null);

This can never change the map, but it currently throws UOE. Should it be changed 
to a no-op as well?


**

What we have now is strict rule: calling mutator methods on unmodifiable 
collections throws an exception. It's simple to describe and reason about, and 
it's (mostly) consistent across various collections. However, it prohibits some 
operations that sometimes people want to do.


If we were to make the system more lenient, or more forgiving, then weird 
inconsistencies and conditional behaviors pop up in other places. This makes the 
system more complicated and harder to reason about, and that leads to more bugs.


s'marks

Re: Kind reminder about JDK-8193031

2019-06-13 Thread Stuart Marks


Hi Sergey,

There are some links to a few other discussion threads in the bug report [1]. 
I've added a link to this one. :-)


It's too late for JDK 13, which entered RDP1 today. However, the JDK 14 source 
base is open, and we can proceed there.


In one of the previous discussions, I had suggested an alternative which is to 
add a default method ("addEach") that adds to a collection all the elements of a 
given array. This was probably a distraction from that discussion, and it might 
have had the side effect of derailing that discussion. So, sorry about that.


Let's focus on Martin's proposal. Basically this replaces the for-loop with code 
that wraps the array with Arrays.asList and then simply calls Collection.addAll. 
There is also some spec cleanup.


Overall I think this is fine, but I did have a reservation. If we have

Collections.addAll(arraylist, array)

this would end up calling

arraylist.addAll(Arrays.asList(array))

The first thing ArrayList.addAll() does is

Object[] a = c.toArray();

which makes a copy of the input array. Right away, this seems like a waste.

On the other hand, I observed that doing two bulk array copies was actually 
faster than making one copy using a for-loop. I don't remember what sizes I 
checked. I'm a bit surprised, though, I would have thought that some sizes would 
be faster and some slower. Perhaps someone should do some more benchmarks. But 
that seems like an advantage of using addAll.


One thing I didn't think of previously was that using a for-loop to add elements 
one-at-a-time might resize the destination ArrayList more than once, resulting 
in some redundant copying, whereas a bulk copy with addAll() will resize the 
destination at most once before doing the copy. That's another point in favor of 
using addAll().


In summary, while this might seem like an obvious thing to do, it's not a 
no-brainer, and there are some subtle tradeoffs. We might end up doing this 
eventually, but it would be good to have a better handle on the tradeoffs and 
potential impact.


Martin was working on this most recently. Martin, what do you think?

s'marks







On 6/13/19 4:39 AM, Сергей Цыпанов wrote:

Hello,

the issue [1] was opened almost two years ago,
but even today Collections.addAll still looks like this:

public static  boolean addAll(Collection c, T... elements) {
 boolean result = false;
 for (T element : elements)
 result |= c.add(element);
 return result;
}

The most wide-spread collections in Java-world is ArrayList for which 
Collections.addAll is likely to be slower then Collection.addAll(Arrays.asList).
The same is valid for ArrayDeque and especially for COWList. There's a webrev 
[2] for mentioned ticket. The whole story begins in [3].

Could the change [2] be done for JDK 13?

Regards,
Sergey Tsypanov


1. https://bugs.openjdk.java.net/browse/JDK-8193031
2. http://cr.openjdk.java.net/~martin/webrevs/jdk/Collections-addAll
3. 
http://mail.openjdk.java.net/pipermail/core-libs-dev/2017-December/050326.html

Re: RFR JDK-8225339 Optimize HashMap.keySet()/HashMap.values()/HashSet toArray() methods

2019-06-13 Thread Stuart Marks


Hi Tagir,

I reviewed your latest changeset and it looks fine.

I ran the changes through our internal test system and the results look good.

Since the CSR was approved for JDK 14, and the mainline is now JDK 14 (after the 
JDK 13 RDP1 fork), you can now push this changeset.


Thanks,

s'marks

On 6/10/19 8:45 PM, Tagir Valeev wrote:

Hello!

I addressed all your suggestions, please check the updated webrev (targeted 
for JDK 14):

http://cr.openjdk.java.net/~tvaleev/webrev/8225339/r3/

As you noted elsewhere, the way the build runs javadoc shouldn't generate
any specification differences, so we don't need to worry about specdiff.
I've applied your patch to my personal tree and I've verified that there
aren't any specification differences. (Indeed, the only difference is that
the HTML files have an additional meta tag; I'm not sure of the
significance of this, but it doesn't seem to impact the specification as
far as I can see.)


Thank you for doing this for me!


Code-related suggestions are addressed in new webrev (along with Roger's
suggestions):
http://cr.openjdk.java.net/~tvaleev/webrev/8225339/r2/


Small item: HashMap.values() and LinkedHashMap.values() still have the
issue where the no-arg toArray() allocates a right-sized array and then
calls toArray(T[]), which then calls prepareArray(), which rechecks the
size. Thanks for changing the other places, but I think these should be
changed too. Unfortunately I think this means introducing a helper method
for values, similar to keysToArray(), and then both toArray() overloads
can call it. But I think this makes good sense and aligns the
implementation with the other toArray() implementations.

I tried to make patch smaller and did not extract valuesToArray because 
there's no need to call it externally (keysToArray should be called from 
HashSet). But you're right, making the code more uniform looks better than 
making the code smaller.


Oh, one more thing in LinkedHashMap.java:

565 public Object[] toArray() { return keysToArray(new Object[size]); }

Please add line breaks.

Oops, missed that. Thanks!

With best regards,
Tagir Valeev

Re: Kind reminder about JDK-8193031

2019-06-15 Thread Stuart Marks


On 6/13/19 7:48 PM, Tagir Valeev wrote:
If ArrayList is the collection which could mostly benefit from this, why not 
peel off this case and provide special path for ArrayList only?


Seems like a reasonable compromise! I'd be interested in what others think. 
Personally I've never used Collections.addAll but I suppose it would be in 
improvement for it to work better (and for its specification not to lie).


s'marks

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

2019-06-27 Thread Stuart Marks


On 6/26/19 9:28 PM, Jaikiran Pai wrote:

I am looking to contribute a patch for the enhancement noted in
https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
looks relatively straightforward to me and here's what I plan to do:

1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
implementing the AutoCloseable interface

2. Have the close() method call the end() method

3. Add javadoc to the close() implementation to mention that the end()
method gets called

4. Add test(s) to verify that the close indeed actually "ends" the
inflater/deflater

5. This is more of a question - do we have to update the class level
javadoc of both these classes to make a mention that these classes have
started implementing the AutoCloseable starting version 14 (or whichever
version this change makes into) of Java?

Any other inputs/suggestions on what else might be needed in this change?


Hi Jaikiran,

Thanks for picking this up. There are some small wrinkles with this, but I hope 
nothing that will prevent it from happening.


1) It's odd that Deflater/Inflater weren't retrofitted to AutoCloseable back in 
Java 7 when AC and try-with-resources were introduced. It might have merely been 
an oversight, as the method is named "end" instead of "close". Joe Darcy, who 
drove much of the retrofitting work, said that he searched for classes that had 
a "close" method to generate the list of retrofit candidates. If so, that would 
have missed the "end" method.


On other hand, there might be some other reason that's not obvious that makes 
retrofitting to AC impractical or incorrect or something. I've asked around 
internally and nobody can think of any fundamental issues, though. Perhaps 
somebody else on the mailing list might know of something?


2) Alan already mentioned this, but we need to consider the issue of idempotent 
close() or end() carefully. It's not strictly required, but it would be nice. I 
had taken a quick look at end() and I *thought* it was idempotent already, but a 
more careful analysis needs to be done. Idempotent close is an issue when using 
multiple, wrapped resources, and where an outer close() closes the inner one as 
well.


If Inflater were retrofitted to implement AC, oddly, the following would not 
require idempotent close:


try (var inf = new Inflater();
 var iis = new InflaterInputStream(inputStream, inf)) {
...
}

The reason is that InflaterInputStream does NOT close the Inflater if it's 
passed one explicitly, but it DOES close it if it created the Inflater itself. 
This makes sense, but unfortunately it doesn't appear to be documented well.
It's probably a potential source of errors in that the doc says "releases any 
system resources associated with the stream" but it doesn't close the Inflater 
if one was passed in explicitly. Oh well, probably too late to change this now.


If we were to make something idempotent, it's not clear whether idempotency 
should apply to just close() or both to end() and close().


3) Good, you're thinking about spec updates. Clearly this will need to reflect 
whatever is decided about idempotency.


Regarding a note in the specification about retrofitting AC, I don't necessarily 
think anything is necessary in the spec itself other than "@since 14" (or 
whatever) on the close() method. There's no @since on an "implements" clause. 
However, a release note would be appropriate for this.


Also, a CSR request should be filed for the spec change and it needs to be a 
approved before the change goes in. This isn't a huge deal but it's required for 
spec changes, it involves some writing, and a few additional days for 
review/approval.


We can help you with release notes and the CSR process when the time comes.

4) Good, you're also thinking about testing. One test you might consider 
updating is test/jdk/java/util/zip/TotalInOut.java.


5) Might be a good idea to update the example code in the class doc to use 
try-with-resources.


Again, thanks for picking this up.

s'marks

Re: RFR (XS) 8224849 : Flag (?U:...) is allowed for non-capturing groups

2019-06-27 Thread Stuart Marks


Hi Ivan,

I think this fix is correct and clearly the spec needed to be fixed. Since it is 
a change to the normative portion of the spec, though, it needs a CSR. This is 
probably mostly a formality to make sure the change is tracked for TCK purposes. 
Can you file a CSR for this please?


Let me know if you need a review for it.

Thanks,

s'marks

Re: Suggestion for a more sensible implementation of EMPTY_MAP

2019-06-27 Thread Stuart Marks





On 6/19/19 12:51 AM, Abraham Marín Pérez wrote:

private void decorate(Map data) {
 //...
 data.computeIfPresent("field", (k, v) -> highlightDifferences(v, 
otherValue));
 //...
}

At one point an emptyMap() was passed to this method, causing an UOE. [...]


On the face of it, the decorate() method has the possibility to modify the map 
that it's passed. Thus, it shouldn't be at all surprising that passing an 
unmodifiable map to it results in UOE.


As a special case, it doesn't *actually* modify the map if "field" is absent, 
but you have to do some analysis to figure this out.


Now you want the JDK to add another special case to Collections.emptyMap(), and 
possibly others, in order to make this special case work. I don't think we want 
to do that.


s'marks

Re: EnumSet.class serialization broken - twice

2019-06-27 Thread Stuart Marks


Arh. Yet another serialization bug.

But yes, this is a bug. Thanks for finding and diagnosing it.

Like Martin, I've often forgotten that classes themselves can be included in a 
serial stream, as well as instances of those classes. In fact I seem to recall 
arguing that because EnumSet uses the serialization proxy pattern, instances of 
it should never appear in a legitimate serial stream. I think that's true. 
However, I sent on to say that because of this, there is no issue with 
serialization compatibility, and thus EnumSet didn't need a serialVersionUID. 
That's incorrect.


I'm uncomfortable with relaxing the serialization spec and mechanism to allow a 
class in the serial stream to have a different svuid from the one loaded in the 
running JVM. Offhand I don't know what problems it could cause, but it seems 
like a fundamental change that would lead to problems at some point.


Also, this is a problem with one class (so far...) and I don't think we should 
change the whole serialization mechanism to support it.


I'm thus leaning toward your first suggestion of adding a serialVersionUID 
declaration to EnumSet that matches the value from JDK 8. This would go into the 
current repo (JDK 14) and likely be backported to JDK 13.


It seems like a no-brainer to backport this to JDK 11 as well; this would 
provide broad compatibility across JDK 8 LTS, JDK 11 LTS, and current JDK 
releases. However, changing the svuid is a specification change. More 
investigation is necessary to figure out what would be involved in doing this.


Meanwhile, it would seem sensible to file a bug and start on a fix for the 
current release(s). Would you be able to do that?


Again, thanks for finding this.

s'marks

On 6/18/19 7:32 AM, Peter Levart wrote:



On 6/18/19 4:00 PM, Martin Buchholz wrote:

Java Historian says:
I was a reviewer for Effective Java 3rd Edition and EnumSet is the canonical 
example of the Serialization Proxy pattern,

so I tried to make sure the pattern was implemented as perfectly as possible.
8192935: Fix EnumSet's SerializationProxy javadoc
All of us who try to make java serialization work right have a mental model of 
the many things that might go wrong.
Serialization of Class objects has never been part of my own mental model - 
I've only ever considered instances.


Perhaps the necessity for Class objects representing Serializable classes to 
agree in sertialVersionUID is a bug in Java serialization implementation? 
There's no such requirement for Class objects representing non-Serializable 
classes and I don't see why this requirement is there for Serializable classes. 
Could this requirement simply be relaxed with no ill consequences?


Regards, Peter




On Tue, Jun 18, 2019 at 5:32 AM Peter Levart > wrote:


    Hi,

    I recently stumbled on an exception thrown when deserializing stream
    produced on JDK 8 and read with JDK 11. I narrowed the problem
    down to
    serialization/deserialization of a public EnumSet.class object. There
    were several changes made to EnumSet in the Mercurial history of jdk
    repo, but I think the following two broke the serialization:

    http://hg.openjdk.java.net/jdk/jdk/rev/d0e8542ef650
    http://hg.openjdk.java.net/jdk/jdk/rev/a7e13065a7a0

    It is interesting to note that before those two changes were made,
    there
    was a chance to fix the problem reported by newly added serial lint
    warnings. Unfortunately they were just silenced:

    http://hg.openjdk.java.net/jdk/jdk/rev/501d8479f798

    +@SuppressWarnings("serial") // No serialVersionUID due to usage of
    +    // serial proxy pattern

    It is true that serialization of instances of Serializable classes is
    not broken by changes to them when they implement serial proxy
    pattern
    (i.e. writeReplace() method) even if they don't itself declare a
    private
    static final long serialVersionUID field, but this is not true of
    Class
    objects representing those Serializable classes. It is even more
    controversial that serialization of Class objects representing
    non-Serializable classes is never broken (which is understandable as
    they don't have a habit of declaring serialVersionUID fields).

    Both of the above braking changes were made post JDK 8 release, so
    deserialization of JDK 8 (and older) streams is affected in all
    JDK 9 +
    releases or vice versa.

    So, what shall be done. I suggest adding serialVersionUID field to
    EnumSet vith a value that corresponds to JDK 8 serialization
    format and
    later backport this change to JDK 11.

    What do you think?


    Regards, Peter


    PS: ImmutableCollections nested classes also implement serial proxy
    pattern and don't declare serialVersionUID fields, but they are not
    public, so it is less chance that Class objects representing them
    could
    be used in serial streams, although it is not impossible. For exa

Re: RFR (XS) 8224849 : Flag (?U:...) is allowed for non-capturing groups

2019-06-28 Thread Stuart Marks


On 6/27/19 2:43 PM, Ivan Gerasimov wrote:

Yes, good point about the spec change.

Here's the CSR filed: https://bugs.openjdk.java.net/browse/JDK-8226914

Can you please review it at your convenience?


Thanks for filing this. I've made some minor edits and have marked it reviewed.

Note that when filing CSRs there are a few fields that need to be filled in but 
that aren't normally visible when viewing the issue; you have to press the Edit 
button to see them. These fields include Compatibility Risk, Compatibility Risk 
Description, Scope, and Reviewed By. I've filled these in appropriately.


I think this is OK for you to mark Finalized now.

Thanks,

s'marks

Re: EnumSet.class serialization broken - twice

2019-06-28 Thread Stuart Marks

Daniel Fuchs pointed me to a weird thing they had to do with the 
serialVersionUID in one of the JMX classes:


https://hg.openjdk.java.net/jdk/jdk/file/c59f36ed7b52/src/java.management/share/classes/javax/management/MBeanAttributeInfo.java#l46

Essentially the svuid for this class initialized in a static block, and its 
value is conditional based on the value of some system property. I don't think 
using a property is necessary for the EnumSet case. However, it does point out 
something interesting, which is that if the svuid is not initialized with a 
compile-time constant, and instead via a static block, the value doesn't appear 
in serialized-form.html.


Thus, we can backport a change to JDK 11 that changes EnumSet's svuid, without 
changing the Java SE 11 specification! (This is analogous to changing a method's 
implementation to behave the way we want to, without changing the method's 
specification to specify that it behaves that way. On the other hand, this is a 
really sleazy hack.)


Here's an outline of what we can do:

1) Add EnumSet.serialVersionUID to JDK 13 in the usual way, specifying a 
constant that's the same as the JDK 8 value. We are after RDP1 but I think this 
change is well-justified. This change should automatically be propagated to JDK 14.


2) "Backport" the fix to JDK 11, but assign the value in a static initializer 
block instead of as a constant in a field initializer. Something like this:



diff -r 27d4b8acbe07 src/java.base/share/classes/java/util/EnumSet.java
--- a/src/java.base/share/classes/java/util/EnumSet.java	Thu Jun 13 17:46:57 
2019 -0700
+++ b/src/java.base/share/classes/java/util/EnumSet.java	Fri Jun 28 16:42:03 
2019 -0700

@@ -76,11 +76,18 @@
  * @since 1.5
  * @see EnumMap
  */
-@SuppressWarnings("serial") // No serialVersionUID due to usage of
-// serial proxy pattern
+@SuppressWarnings("serial") // serialVersionUID is not a compile-time constant
 public abstract class EnumSet> extends AbstractSet
 implements Cloneable, java.io.Serializable
 {
+// Initialize the serialVersionUID to be compatible with JDK 8.
+// Do this from a static block to avoid having the value appear
+// in serialized-form.html. See JDK-xxx.
+private static final long serialVersionUID;
+static {
+serialVersionUID = 1009687484059888093L;
+}
+
 /**
  * The class of all the elements of this set.
  */

3) Backport the fix to JDK 12. I'm not sure this is absolutely necessary, since 
JDK 12 likely has a short lifetime, but if we're doing 11 and 13 it makes sense 
to do 12 as well (though mainly for completeness).


4) It's unclear whether a similar patch as above needs to be added to JDK 8. 
Since it already has the right svuid, we could get away without doing anything. 
However, with backports continuing in the 8u release family, it might be prudent 
to apply this patch in order to prevent future backports to 8u from 
inadvertently changing the svuid -- which as you point out did happen in 9 and 10.


5) I don't think we need to patch JDK 1.6, 7, 9, or 10 but the maintainers of 
those releases can certainly decide to do so.


=

In any case, I think doing the above will result in consistent 
EnumSet.serialVersionUID values for JDK 8 LTS, JDK 11 LTS, and current JDK 
releases, without having to worry about spec changes for any of the past releases.


Let me know how you'd like to proceed. I'm happy to help out with reviewing, 
filing bugs, CSRs, etc.


s'marks






On 6/27/19 2:57 PM, Stuart Marks wrote:

Arh. Yet another serialization bug.

But yes, this is a bug. Thanks for finding and diagnosing it.

Like Martin, I've often forgotten that classes themselves can be included in a 
serial stream, as well as instances of those classes. In fact I seem to recall 
arguing that because EnumSet uses the serialization proxy pattern, instances of 
it should never appear in a legitimate serial stream. I think that's true. 
However, I sent on to say that because of this, there is no issue with 
serialization compatibility, and thus EnumSet didn't need a serialVersionUID. 
That's incorrect.


I'm uncomfortable with relaxing the serialization spec and mechanism to allow a 
class in the serial stream to have a different svuid from the one loaded in the 
running JVM. Offhand I don't know what problems it could cause, but it seems 
like a fundamental change that would lead to problems at some point.


Also, this is a problem with one class (so far...) and I don't think we should 
change the whole serialization mechanism to support it.


I'm thus leaning toward your first suggestion of adding a serialVersionUID 
declaration to EnumSet that matches the value from JDK 8. This would go into the 
current repo (JDK 14) and likely be backported to JDK 13.


It seems like a no-brain

Re: Inputs on patch for JDK-8225763? Inflater and Deflater should implement AutoCloseable

2019-07-02 Thread Stuart Marks

t be a RuntimeException, since most methods don't declare any 
checked exceptions.


In any case, the clause would have to be updated to say something like "Once 
this Deflater (Inflater) is closed, any subsequent operations will ."


**

If you think the issues are settled enough, maybe it's time for you to take a 
stab at a patch. Up to you how to proceed with the "undefined" issue. If it's 
simple, great, I'd like to see it happen. But if it leads you off into the 
weeds, then feel free to drop it.


Note: US holiday weekend coming up; replies might take several days.

s'marks




On 6/29/19 4:16 AM, Jaikiran Pai wrote:


On 29/06/19 4:31 PM, Jaikiran Pai wrote:

Hello Stuart,

Thank you for the detailed response. Comments inline.

On 28/06/19 2:48 AM, Stuart Marks wrote:

On 6/26/19 9:28 PM, Jaikiran Pai wrote:

I am looking to contribute a patch for the enhancement noted in
https://bugs.openjdk.java.net/browse/JDK-8225763. The change itself
looks relatively straightforward to me and here's what I plan to do:

1. Have both java.util.zip.Inflater and java.util.zip.Deflater start
implementing the AutoCloseable interface

2. Have the close() method call the end() method

3. Add javadoc to the close() implementation to mention that the end()
method gets called

4. Add test(s) to verify that the close indeed actually "ends" the
inflater/deflater

5. This is more of a question - do we have to update the class level
javadoc of both these classes to make a mention that these classes have
started implementing the AutoCloseable starting version 14 (or whichever
version this change makes into) of Java?

Any other inputs/suggestions on what else might be needed in this
change?

Hi Jaikiran,

Thanks for picking this up. There are some small wrinkles with this,
but I hope nothing that will prevent it from happening.


2) Alan already mentioned this, but we need to consider the issue of
idempotent close() or end() carefully. It's not strictly required, but
it would be nice. I had taken a quick look at end() and I *thought* it
was idempotent already, but a more careful analysis needs to be done.

I did some checks in the current JDK code. From what I see, the Inflater
and Deflater do not have any subclasses within the JDK itself.


To be clear - I couldn't find any subclasses in the main source code of
JDK. There are a couple of subclasses in the tests
(ConstructInflaterOutput.java and ConstructDeflaterInput.java), but
those don't do much outside of the context of testing.

-Jaikiran

Re: EnumSet.class serialization broken - twice - JDK-8227368

2019-07-08 Thread Stuart Marks


Hi Peter,

Thanks for picking this up, for filing the bug (JDK-8227368), and developing the 
fix.


For the current release, I think we should go ahead and put in the "right" fix, 
that is, to define serialVersionUID the conventional way -- as a compile-time 
constant -- and then simply to remove the suppression of the serial warning. 
This will change the specification (because the change will appear in 
serialized-form.html), but that's OK, we have the ability to change the 
specification. Note that this will require a CSR. (Changing the svuid value 
without changing the specification would probably require a CSR anyway, since 
it's a behavioral change that affects compatibility.) Might as well fix the 
specification now and be done with it, at least for current and future releases.


The JDK 11 backport is where we should apply the static initializer. That's 
where changing the specification is more difficult. I think we should discuss 
that separately, though.


Thanks for writing the new test and updating the BogusEnumSet test.

Given that this is a fix for an incompatible change, I think this should be 
fixed as soon as possible. I've raised the priority to P3, and I've targeted 
this bug to JDK 13. We're after RDP1, but important fixes can still go in during 
RDP1 [1] until the next phase, RDP2, which begins on July 18. [2] Once this goes 
into JDK 13, it should automatically be propagated to JDK 14, with no manual 
steps necessary.


With the fairly short time frame left in JDK 13 before RDP2, it would be best to 
move as quickly as possible. If you have time to work in this immediately, 
great, but if not, that's ok; please let me know and I'll pick up any or all 
work that you don't have time to do. Regardless, I'll help out any way I can, 
such as reviewing the code and the CSR, doing testing, etc.


Next steps:

1. New changeset with constant version of EnumSet.serialVersionUID.

2. Create draft CSR.

Thanks,

s'marks


[1] http://openjdk.java.net/jeps/3

[2] http://openjdk.java.net/projects/jdk/13/




On 7/7/19 8:11 AM, Peter Levart wrote:

On 7/7/19 4:31 PM, Peter Levart wrote:

On 6/29/19 2:00 AM, Stuart Marks wrote:
Daniel Fuchs pointed me to a weird thing they had to do with the 
serialVersionUID in one of the JMX classes:


https://hg.openjdk.java.net/jdk/jdk/file/c59f36ed7b52/src/java.management/share/classes/javax/management/MBeanAttributeInfo.java#l46 



Essentially the svuid for this class initialized in a static block, and its 
value is conditional based on the value of some system property. I don't 
think using a property is necessary for the EnumSet case. However, it does 
point out something interesting, which is that if the svuid is not 
initialized with a compile-time constant, and instead via a static block, the 
value doesn't appear in serialized-form.html. 


Hi Stuart,

I just realized that I was reading your last statement wrong. I though it was 
written as:


...if the svuid is not initialized with a compile-time constant, and instead via 
a static block, the value doesn't appear in serialized form.


And not as:

...if the svuid is not initialized with a compile-time constant, and instead via 
a static block, the value doesn't appear in serialized-form.html



You were only concerned about the generated javadoc and not about the actual 
"serialized form". Ok, I get it now. I have prepared webrev.02 to fix this.



Regards, Peter

Re: EnumSet.class serialization broken - twice - JDK-8227368

2019-07-09 Thread Stuart Marks


1. New changeset with constant version of EnumSet.serialVersionUID.


This is already in the webrev.01 changeset. webrev.02 is an attempt to sneak 
the change without being visible in the serialized-form.html.


Ah. I skipped webrev.01 because I thought that webrev.02 had superseded it. 
Looking at webrev.01, I see



+// value computed from JDK 8 (and previous) EnumSet class
+// needed to properly cross-(de)serialize EnumSet.class objects
+// between JDK 8- <-> JDK 9+
+private static final long serialVersionUID = 1009687484059888093L;
+


I don't think this comment can cover the entire history here. We'll have to rely 
on the bug report, the CSR, and the email archives.


Most declarations of serialVersionUID don't have a comment at all. So, we could 
just omit it.


If you feel a comment is necessary, perhaps something like

// declare serialization compatibility with JDK 8 (see JDK-8227368)

might be sufficient. Otherwise, webrev.01 looks fine.


On 7/9/19 1:57 AM, Stuart Marks wrote:
2. Create draft CSR. 


Created:

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


I've done some editing on this CSR and I've marked myself as a reviewer. Please 
move this to Finalized.


While we're waiting for the CSR to be approved (I hope this takes only a day or 
two) I'll do some testing with your patch.


Thanks,

s'marks

1 2 3 4 5 6 7 8 9 10 >

1 - 100 of 1414 matches

Mail list logo