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

[Stuart Marks]

On Sat, 21 Nov 2020 11:02:27 GMT, Rémi Forax 
 wrote:

>> 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.
>
> src/java.base/share/classes/java/util/ImmutableCollections.java line 222:
> 
>> 220: default:
>> 221: return (List) new ListN<>(input, false);
>> 222: }
> 
> Using a switch expression + arrow (->) here will allow you too factor the 
> cast, even if it disappear in the generated bytecode, I think it makes the 
> code more readable

Heh yes, I haven't gotten used to using those yet!

-

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

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

[Stuart Marks]

On Sat, 21 Nov 2020 10:58:49 GMT, Rémi Forax 
 wrote:

>> 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.
>
> you can still calls the varargs with an already created array
> listFromTrustedArray(new String[] { "foo" });
> 
> I think at least an assert is missing
> assert input.getClass() == Object.class;

If the parameter were of type `E...`, then a call such as 
`listFromTrustedArray("foo", "bar")` would result in a `String[]`. The idea is 
to avoid accidents such as the one that caused JDK-6260652, and not prevent 
deliberate passing of an array of the wrong type, though this is an internal 
interface so it's unlikely to occur. Still, a stronger comment and an assert 
might be worthwhile.

-

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

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

[Stuart Marks]

On Mon, 23 Nov 2020 14:06:02 GMT, sergus13 
 wrote:

>> This implementation duplicates the array twice, once in this.toArray() and 
>> once in the constructor of ArrayList that calls toArray() on 
>> Collections.ArrayList.
>> 
>> I'm sure there is a better way
>
> In `ReferencePipeline` class we have:
> 
> @Override
> public final Object[] toArray() {
> return toArray(Object[]::new);
> }
> 
> @Override
> @SuppressWarnings("unchecked")
> public final  A[] toArray(IntFunction generator) {
> ...
> @SuppressWarnings("rawtypes")
> IntFunction rawGenerator = (IntFunction) generator;
> return (A[]) Nodes.flatten(evaluateToArrayNode(rawGenerator), 
> rawGenerator)
>   .asArray(rawGenerator);
> }
> 
> 
> In `Nodes` class we have:
> 
> 
> public static  Node flatten(Node node, IntFunction 
> generator) {
> if (node.getChildCount() > 0) {
> ... 
> T[] array = generator.apply((int) size);
> new ToArrayTask.OfRef<>(node, array, 0).invoke();
> return node(array);
> } else {
> return node;
> }
> }
> 
> 
> 
> It looks like it is required to implement `toList` method in a similar way in 
> order to avoid array copy. i.e. there will be `IntFunction> 
> generator` which will generate 'ArrayList' with specified size and the list's 
> `add` method will be called to add elements to the list.

This is the default implementation in the Stream interface, which is overridden 
by an implementation in the ReferencePipeline class, so it will rarely be used 
in normal operation. The ReferencePipeline version (part of this changeset) is 
based on toArray() and avoids any copying. I'm thus not inclined to add new 
interfaces in order to support this default implementation.

As written it's true that the default implementation does perform apparently 
redundant copies, but we can't be assured that toArray() actually returns a 
freshly created array. Thus, we wrap it using Arrays.asList and then copy it 
using the ArrayList constructor. This is unfortunate but necessary to avoid 
situations where someone could hold a reference to the internal array of a 
List, allowing modification of a List that's supposed to be unmodifiable.

-

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

Re: RFR: 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed,… [v2]

[Stuart Marks]

On Fri, 20 Nov 2020 19:59:52 GMT, Mandy Chung  wrote:

>> Alan Bateman has updated the pull request with a new target base due to a 
>> merge or a rebase. The incremental webrev excludes the unrelated changes 
>> brought in by the merge/rebase. The pull request contains four additional 
>> commits since the last revision:
>> 
>>  - Fixed typo in @deprecated text
>>  - Merge
>>  - Update jshell class
>>  - 8256643: Terminally deprecate ThreadGroup stop, destroy, isDestroyed, 
>> setDaemon and isDaemon
>
> Marked as reviewed by mchung (Reviewer).

I think the current deprecation wording is actually too specific regarding the 
raciness between TG destruction and created-but-not-started threads. That's 
just one of the flaws of thread groups. In fact, I think there are enough 
weirdnesses and race conditions around all destruction-related operations of 
thread groups that the whole concept is fundamentally flawed. We should just 
say that. How about this:

> ThreadGroup's destruction mechanisms are fundamentally flawed. Therefore, the 
> ThreadGroup methods destroy(), isDestroyed(), setDaemon(), and isDaemon(), 
> which relate to ThreadGroup destruction, have been deprecated and may be 
> removed from a future version of the system.

I think there are too many subtle details to include a justification here about 
why TG destruction is fundamentally flawed, so we just have to assert that. 
Unfortunately the writeups in the JEP and CSR are in draft state so we can't 
link to them. Maybe when the JEP is published we can add a link to it from here 
later.

-

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

Integrated: 8037384: Fix wording in Javadoc of java.io.Serializable

[Stuart Marks]

On Thu, 19 Nov 2020 03:44:10 GMT, Stuart Marks  wrote:

> 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.

This pull request has now been integrated.

Changeset: cc0ed401
Author:Stuart Marks 
URL:   https://git.openjdk.java.net/jdk/commit/cc0ed401
Stats: 27 lines in 5 files changed: 5 ins; 1 del; 21 mod

8037384: Fix wording in Javadoc of java.io.Serializable
8231547: Serializable class doc should link to serialization specification

Reviewed-by: rriggs, iris, chegar

-

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

Re: RFR: 8037384: Fix wording in Javadoc of java.io.Serializable [v2]

[Stuart Marks]

On Thu, 19 Nov 2020 17:11:04 GMT, Chris Hegarty  wrote:

>> Stuart Marks has updated the pull request incrementally with one additional 
>> commit since the last revision:
>> 
>>   non-serializable classes => non-serializable superclasses
>
> src/java.base/share/classes/java/io/Serializable.java line 50:
> 
>> 48:  * fields of non-serializable classes. During deserialization, the 
>> fields of
>> 49:  * non-serializable classes will be initialized using the no-arg 
>> constructor of
>> 50:  * the class. This constructor must be accessible to the subclass that 
>> is being
> 
> .. of the first non-serializable superclass.

I updated these to say "superclasses" plus a bit of other minor adjustment.

-

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

Re: RFR: 8037384: Fix wording in Javadoc of java.io.Serializable [v2]

[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.

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

  non-serializable classes => non-serializable superclasses

-

Changes:
  - all: https://git.openjdk.java.net/jdk/pull/1306/files
  - new: https://git.openjdk.java.net/jdk/pull/1306/files/ce7e6d6c..54c17ba6

Webrevs:
 - full: https://webrevs.openjdk.java.net/?repo=jdk=1306=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1306=00-01

  Stats: 3 lines in 1 file changed: 0 ins; 0 del; 3 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

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

[Stuart Marks]

On Thu, 19 Nov 2020 00:58:21 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:
> 
>   Comment cleanup

Marked as reviewed by smarks (Reviewer).

-

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

RFR: 8037384: Fix wording in Javadoc of java.io.Serializable

[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=1306=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

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

[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

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

[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=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=1026=02
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1026=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

Integrated: 8256152: tests fail because of ambiguous method resolution

[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 [v2]

[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

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

[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: 8256152: tests fail because of ambiguous method resolution [v2]

[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=1274=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1274=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: 8180352: Add Stream.toList() method [v2]

[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

RFR: 8256152: tests fail because of ambiguous method resolution

[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=1274=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]

[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

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

[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=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=1026=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=1026=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

[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=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

[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=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

[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=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

[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=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

[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=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

[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

[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

[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=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

[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=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

[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=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

RFR: 8180352: Add Stream.toList() method

[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=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=1026=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: 8188055: (ref) Add Reference::refersTo predicate [v6]

[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

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

[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

Integrated: 8255016: ConstantDescs.FALSE claims it represents TRUE

[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

RFR: 8255016: ConstantDescs.FALSE claims it represents TRUE

[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=744=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

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

[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

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

[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 8253451: Performance regression in java.util.Scanner after 8236201

[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: 8252999: Cleanup: replace .equals("") with .isEmpty() within all codebase

[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: 'Find' method for Iterable

[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: 8252730: jlink does not create reproducible builds on different servers

[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

Integrated: 8253066: typo in Stream.mapMulti

[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

RFR: 8253066: typo in Stream.mapMulti

[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=137=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

Re: RFR: 8251495: Clarify DOM documentation

[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

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

[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: 8157729: examples in LinkedHashMap and LinkedHashSet class doc use raw types [v2]

[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=87=01
 - incr: https://webrevs.openjdk.java.net/?repo=jdk=87=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

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

[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

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

[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=87=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: 8252830: Correct missing javadoc comments in java.rmi module [v3]

[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

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

[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 [15/java.xml] 8248884: Regression caused by the update to BCEL 6.0

[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: RFR [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[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 [16/java.xml] 8248348: Regression caused by the update to BCEL 6.0

[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: Sometimes constraints are questionable

[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: Final call Re: RFR: JDK-8230744 Several classes throw OutOfMemoryError without message

[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/~jlaskey/8230744/webrev-01/index.html 
<http://cr.openjdk.java.net/~jlaskey/8230744/webrev-01/

Re: Sometimes constraints are questionable

[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: Sometimes constraints are questionable

[Stuart Marks]

ion 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 PriorityBlockingQueue and ArrayDeque 
to use ArraysSupport.newLength, in order to provide a consistent policy for 
collections. Other growable-array-based coll

Re: Sometimes constraints are questionable

[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

[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 

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

[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: 8246183: Scanner/ScanTest.java fails due to SIGSEGV in StubRoutines::jshort_disjoint_arraycopy

[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 15 (S): 8245068: Implement Deprecation of RMI Activation

[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: RFR 15 (S): 8245068: Implement Deprecation of RMI Activation

[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

[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

[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

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

[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] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[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

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

[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

[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

[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

[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]

*This class is /not/ a general-purpose |Map| implementation! While this class 
implements the |Map| interface, it intentionally violates |Map's| general 
contract, which mandates the use of the |equals| method when comparing objects. 
This class is designed for use only in the rare cases wherein reference-equality 
semantics are required.*


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

*While the object returned by this method implements the |Collection| interface, 
it does /not/ obey |Collection's| general contract. Like its backing map, the 
collection returned by this method defines element equality as 
reference-equality rather than object-equality. This affects the behavior of its 
|contains|, |remove| and |containsAll| methods.*


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=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

[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 seemed like the safest 
and most minimal thing to do.


In fact, I'm not even sure how s

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

[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=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=9YMcjPnAAEf40XGqmcEU_Uiyg_xCVlOU26-U56hEb4A=ueOfMpgrTsTZc7xg22AdzVskohCya_stq_WLTREzw24=

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=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4=
 .
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: [15] RFR: 8244459: Optimize the hash map size in LocaleProviderAdapters

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

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

[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: RFR [15] 6394757: rev2: AbstractSet.removeAll semantics are surprisingly dependent on relative sizes

[Stuart Marks]

. 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: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

[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: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException

[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=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4= 
.

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

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

[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

[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=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=OXEC-w1xTWoJUyw_sRivRwIGmkHIUc3DllMsA_N3qIk=iesFFcE4NSOreLmf8vwdKzcdnFgjGE_SEERICPUFef4=
 .
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: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

[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: 7147994 Hashtable rehash‎(‎) javadoc describes implementation details

[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=DwICaQ=jf_iaSHvJObTbx-siA1ZOg=rA_13vli8clPM_toR46hq8FVwH3XGr8z7cUfcQoqL-k=bNY044TQSELJWMafSRxC-62CyHMVnMxboXMG0qqU6CU=V9rN7MCO4rArlV8tRjEn44_Kl_tCH2h-xjqakCvpBxY=
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

[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: 6415694 Clarification in Javadoc for java.rmi.AlreadyBoundException

[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: Collections.synchronizedXXX() and internal mutex (aka SyncRoot)

[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: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

[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: LinkedHashMap/LinkedHashSet enhancement: OrderedMap/OrderedSet

[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

[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 first or null if empty
   E pollFirst();
   // Retrieve 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 iteratorFrom and 

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

[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: Some Classes with a public void close() don't implement AutoCloseable

[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: RFR 15 8242327: List spec should state that unmodifiable lists implement RandomAccess

[Stuart Marks]

Thanks Lance.

Upon advice from Pavel Rappo, I've adjusted the link markup to use the full 
reference form of a javadoc link. Revised patch below. Also, could you review 
the CSR?


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

Thanks,

s'marks



# HG changeset patch
# User smarks
# Date 1586486847 25200
#  Thu Apr 09 19:47:27 2020 -0700
# Node ID 45a22ec2df1a3738d241d82c375507bf4aeb3d1b
# Parent  46108b5b69d92dd424b73b828454849c397509cb
8242327: List spec should state that unmodifiable lists implement RandomAccess
Reviewed-by: XXX

diff -r 46108b5b69d9 -r 45a22ec2df1a 
src/java.base/share/classes/java/util/List.java
--- a/src/java.base/share/classes/java/util/List.java   Tue Apr 07 16:31:46 
2020 -0700
+++ b/src/java.base/share/classes/java/util/List.java   Thu Apr 09 19:47:27 
2020 -0700
@@ -104,6 +104,8 @@
  * They are serializable if all elements are serializable.
  * The order of elements in the list is the same as the order of the
  * provided arguments, or of the elements in the provided array.
+ * The lists and their {@link #subList(int, int) subList} views implement 
the
+ * {@link RandomAccess} interface.
  * They are value-based.
  * Callers should make no assumptions about the identity of the returned 
instances.

  * Factories are free to create new instances or reuse existing ones. 
Therefore,




On 4/8/20 2:33 PM, Lance Andersen wrote:

Hi Stuart,

This looks good

On Apr 8, 2020, at 5:01 PM, Stuart Marks <mailto:stuart.ma...@oracle.com>> wrote:


Hi all,

Please review this small change to the List interface that specifies that 
unmodifiable lists (returned by List.of et al) implement the RandomAccess 
marker interface. In fact, they already do; the intent of this section was to 
specify the user-visible characteristics of such lists, but this particular 
characteristic was accidentally omitted.


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

Patch below. I'll follow up with a CSR request.

Thanks,

s'marks

RFR 15 8242327: List spec should state that unmodifiable lists implement RandomAccess

[Stuart Marks]

Hi all,

Please review this small change to the List interface that specifies that 
unmodifiable lists (returned by List.of et al) implement the RandomAccess marker 
interface. In fact, they already do; the intent of this section was to specify 
the user-visible characteristics of such lists, but this particular 
characteristic was accidentally omitted.


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

Patch below. I'll follow up with a CSR request.

Thanks,

s'marks


# HG changeset patch
# User smarks
# Date 1586379408 25200
#  Wed Apr 08 13:56:48 2020 -0700
# Node ID f0b78a923c2b2eadfc79cfec7f65377588f10574
# Parent  46108b5b69d92dd424b73b828454849c397509cb
8242327: List spec should state that unmodifiable lists implement RandomAccess
Reviewed-by: XXX

diff -r 46108b5b69d9 -r f0b78a923c2b 
src/java.base/share/classes/java/util/List.java
--- a/src/java.base/share/classes/java/util/List.java   Tue Apr 07 16:31:46 
2020 -0700
+++ b/src/java.base/share/classes/java/util/List.java   Wed Apr 08 13:56:48 
2020 -0700
@@ -104,6 +104,7 @@
  * They are serializable if all elements are serializable.
  * The order of elements in the list is the same as the order of the
  * provided arguments, or of the elements in the provided array.
+ * The lists and their {@link subList subList} views implement the {@link 
RandomAccess} interface.

  * They are value-based.
  * Callers should make no assumptions about the identity of the returned 
instances.

  * Factories are free to create new instances or reuse existing ones. 
Therefore,

Re: JDK 15 RF(pre)R of JDK-8241374: add Math.absExact

[Stuart Marks]

Hi Joe,

Overall this looks quite good. Thanks for being thorough about this; I certainly 
would have forgotten about StrictMath. :-) Since this is integer arithmetic, is 
it true that the StrictMath versions are identical to the Math versions?


I have only a couple editorial quibbles.


+ * {@code int} range and an exception is thrown for that argument.


Recommend adding a comma here:


+ * {@code int} range, and an exception is thrown for that argument.


--


+ * @return the absolute value of the argument absent overflow


I stumbled while reading this. While not incorrect, it seems a bit awkwardly 
worded to me. How about something like "...the argument, unless overflow occurs."


--


+throw new ArithmeticException("Cannot represent " +
+  "absolute value of " +
+  "Integer.MIN_VALUE");


In cases like this I usually prefer to put the entire string on its own line, 
avoiding the concatenation:



+throw new ArithmeticException(
+"Cannot represent absolute value of Integer.MIN_VALUE");


--

Thanks,

s'marks




On 3/28/20 1:34 PM, Joe Darcy wrote:

Hello,

Please review the initial proposed wording of the spec for

     JDK-8241374: add Math.absExact

The eventual wording needs to be replicated four times (Math.absExact(int), 
Math.absExact(long), StrictMath.absExact(int), StrictMath.absExact(long)) so I 
want to get the wording agreed to before presenting it in quadruplicate.


The other "exact" methods mention overflow so I wanted the spec to absExact to 
include both "exact" and "overflow" in its description.


Tests will follow in subsequent review iterations.

Thanks,

-Joe

diff -r c5d90e8d4a46 src/java.base/share/classes/java/lang/Math.java
--- a/src/java.base/share/classes/java/lang/Math.java    Sat Mar 28 11:00:09 
2020 -0400
+++ b/src/java.base/share/classes/java/lang/Math.java    Sat Mar 28 10:17:39 
2020 -0700

@@ -1369,6 +1369,32 @@
  }

  /**
+ * Returns the mathematical absolute value of an {@code int} value
+ * if it is exactly representable as an {@code int}, throwing
+ * {@code ArithmeticException} if the result overflows the
+ * positive {@code int} range.
+ *
+ * Since the range of two's complement integers is asymmetric
+ * with one additional negative value, the mathematical absolute
+ * value of {@link Integer#MIN_VALUE} overflows the positive
+ * {@code int} range and an exception is thrown for that argument.
+ *
+ * @param   a   the argument whose absolute value is to be determined
+ * @return the absolute value of the argument absent overflow
+ * @throws ArithmeticException if the argument is {@link Integer#MIN_VALUE}
+ * @see Math#abs(int)
+ * @since 15
+ */
+    public static int absExact(int a) {
+    if (a == Integer.MIN_VALUE)
+    throw new ArithmeticException("Cannot represent " +
+  "absolute value of " +
+  "Integer.MIN_VALUE");
+    else
+    return abs(a);
+    }
+
+    /**
   * Returns the absolute value of a {@code long} value.
   * If the argument is not negative, the argument is returned.
   * If the argument is negative, the negation of the argument is returned.

Re: RFR [15] 8161558: ListIterator should not discard cause on exception

[Stuart Marks]

On 2/25/20 8:01 AM, fo...@univ-mlv.fr wrote:

If the IOOBE is noise, it can be ignored.



You are forcing all primary users to ignore something that is useful only for 
the secondary users.
Given that the ratio between primary vs secondary users is at least 100 000 for 
1 if not more, that's a lot of noise to filter.
I don't think chaining exception IOOBE to a NSEE worth the cost here.


Hi Rémi,

I really do not see the cost here. Most information in an exception (such as the 
stack trace itself, including classes, methods, and line numbers, along with any 
chained exceptions) is about implementation. Perhaps most users don't care about 
this. However, if you're debugging a system, you care about the implementation 
details, and this information is essential. In fact, the more information there 
is, the better. You're effectively arguing to throw stuff away. Continuing this 
line of reasoning, maybe we should discard the stack traces below the public API 
boundary, since most users don't care about it, since it's just internal 
implementation details of the JDK.


No, I don't think so.

s'marks

Re: ZoneRules.of() implies that transitionList is a superset of standardOffsetTransitionList but doesn't check that

[Stuart Marks]

Was any bug ever filed for this? I looked in JBS and I couldn't find anything 
that resembled the issue here.


Roman, since you don't have an account on JBS (bugs.openjdk.java.net), I'd 
suggest you file a bug via bugs.java.com. Bugs filed there do eventually get 
into JBS. (I believe it's difficult for submitters to track how this happens, 
but it does work.)


But earlier, you had said,


java.bugs.com you mean? Not long ago I reported a similar issue about core
libs and they recommended me to write to list directly, so that's why I
wrote directly here this time.


Who said that? That's strange. The preferred route for people who don't have JBS 
to submit bugs against the JDK is indeed bugs.java.com. Please contact us if it 
happens again.


Of course if you have questions or relevant issues to discuss, you're always 
welcome to come here or to other OpenJDK mailing lists.


s'marks



On 1/31/20 10:05 AM, Roman Leventov wrote:

I cannot create an issue because I'm not an OpenJDK process member.

Yes, the test constructs an invalid ZoneRules, the point is to demonstrate
that this is possible and IAE is not thrown upon construction. I don't
think this is a good principle for immutable classes. Once they are
constructed, their state must be verified to be correct.

On Fri, 31 Jan 2020 at 20:17,  wrote:


I meant the JDK Bug System:

https://bugs.openjdk.java.net/

All the issues in the JDK is tracked here.

As to the issues themselves, you would want to specify
Arrays.asList(trans) in place for the 4th argument on ZoneRules.of(), so
that the transition happens on the transition day?

For 2), good catch for the missing 'i', we need a JBS issue for that to
fix it.

Naoto

On 1/31/20 6:08 AM, Roman Leventov wrote:

java.bugs.com  you mean? Not long ago I reported

a

similar issue about core libs and they recommended me to write to list
directly, so that's why I wrote directly here this time.

On Fri, 31 Jan 2020 at 16:57, mailto:naoto.s...@oracle.com>> wrote:

 Hi Roman,

 Please file a JBS issue for this.

 Naoto

 On 1/29/20 9:56 PM, Roman Leventov wrote:
  > 1) ZoneRules.of() implies that transitionList is a superset of
  > standardOffsetTransitionList but doesn't check that. Then it's
 possible to
  > construct ZoneRules instances that don't work correctly:
  >  @Test
  >  public void zoneRulesTest() {
  >  LocalDateTime transitionDay = LocalDateTime.of(2020, 1,
 1, 2, 0);
  >  ZoneOffsetTransition trans = ZoneOffsetTransition.of(
  >  transitionDay,
  >  ZoneOffset.ofHours(1),
  >  ZoneOffset.ofHours(2)
  >  );
  >  ZoneRules rules = ZoneRules.of(ZoneOffset.ofHours(1),
  > ZoneOffset.ofHours(1),
  >  Arrays.asList(trans),
  >  Collections.emptyList(),

Collections.emptyList());

  >
  >  Assert.assertEquals(ZoneOffset.ofHours(2),

rules.getOffset(

  >
 transitionDay.plusDays(7).toInstant(ZoneOffset.UTC)));
  >  }
  >
  > 2) Unrelated issue in java-time code: in
 ZoneOffsetTransitionRule, there is
  > a typo, some variables are called "timeDefnition" (missed "i")
 and it leaks
  > to public Javadoc.
  >

Re: RFR [15] 8161558: ListIterator should not discard cause on exception

[Stuart Marks]

On 2/14/20 2:34 PM, fo...@univ-mlv.fr wrote:

The thing is that inside the iterator, you already have the right information, 
so you don't have to pray to have the right info.


No, the iterator only can guess at the reason that get() threw the exception.


I just disagree on the conclusion, chaining exception is great when you bubble 
up an information that make sense to the end-user,
knowing that next() is implemented using get() that why you get an IOOBE as 
cause of the NSEE is just noise for a end-user.


If the IOOBE is noise, it can be ignored. But suppose the programmer is 
implementing a list by subclassing AbstractList, and they have some internal 
inconsistency that causes their code to throw IOOBE unexpectedly. AbstractList 
does this programmer a disservice by THROWING AWAY the nice stack trace and 
message generated by the subclass or possibly by the VM.


Recall that as an abstract class, AbstractList has end users (callers) as well 
as subclassers, and it needs to provide services to both. Chaining the 
exceptions is a service to subclass implementors.


s'marks

Re: RFR [15] 8161558: ListIterator should not discard cause on exception

[Stuart Marks]

On 2/12/20 2:04 PM, fo...@univ-mlv.fr wrote:

I don't disagree with the fact that having the index may help,
I disagree with the fact that chaining the IOOBE to the NSEE is the right way 
to expose that index, crafting an error message with the index is IMO a better 
idea because you have no idea if the exception thrown by all implementation of 
List.get() will provide this index (and only this index).


It /seems/ like AbstractList has enough information to do the checks itself and 
to formuate a reasonable error message, but it really doesn't. It does need to 
delegate to the subclass's get() implementation to do the actual work. The get() 
method has only two choices: it can return a valid element, or it can throw 
IOOBE. AbstractList can guess at why get() failed, but the knowledge of that 
belongs to the subclass, and it's the subclass's responsibility to put that 
information into the IOOBE.


Of course it's possible that some subclass hasn't provided useful information in 
the exception, but that shouldn't be a reason to penalize subclasses that have 
done so.



It depends how List.get() is implemented, if there is a bound check before 
accessing the value in the implementation, you have the same issue, one turtle 
down :)


It's a subclass implementation choice as to whether it wants to do a bounds 
check and throw explicitly or to let the VM generate the exception implicitly. 
Either way results in an IOOBE (or some subtype). Which of these is done is 
irrelevant to AbstractList; all it knows is that get() reported an error by 
throwing an exception of this type, so it should be chained into the resulting NSEE.


s'marks





s'marks


Rémi



On 2/5/20 2:05 PM, Remi Forax wrote:

Stuart, Martin, Kiran,
I think this "bug" should not be fixed because it's one of the cases where
providing more information is actually bad from a user POV.

The current code throws NoSuchElementException when the iterator reach the end
so from the user POV, this is the right exception because of the right issue,
so from the user POV there is no need to change the actual code.
If we chain the exception,
- it's less clear from a user POV
- the user may think that there is an error in the AbstractList implementation
but it's not the case, it's just that AbstractList iterators next method is
implemented weirdly, it prefers to go out of bound instead of checking the
bound.

I'm okay with NoSuchElementException having a new constructor that takes a
chained exception but i really think that in this specific case, it's a bad
idea(TM) to use it to propagate an exception to the user that it should not
care about.

BTW, perhaps, those method next() should be re-written to test the bound instead
of catching the IOOBE because i'm not sure this "optimization" make sense
nowadays.

regards,
Rémi

- Mail original -

De: "Kiran Ravikumar" 
À: "core-libs-dev" 
Envoyé: Mercredi 5 Février 2020 20:49:09
Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on exception



Thanks Stuart and Martin,


Here is an updated webrev with the changes.

http://cr.openjdk.java.net/~kravikumar/8161558/webrev.01/

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


Thanks,

Kiran


On 15/01/2020 12:46, Martin Buchholz wrote:

Hi Kiran,

Looks good to me, but I always nitpick ...

Stray semicolon?
         var iterator = list.listIterator(list.size());; // position at end

I would have documented whitebox test assumptions: that nCopies
iterators are implemented via AbstractList, and that
AbstractList's list iterator inherits behavior from iterator.

I probably would have added a plain iterator test, and might have
refactored the code that tests the exception.


On Wed, Jan 15, 2020 at 4:07 AM Kiran Ravikumar
mailto:kiran.sidhartha.raviku...@oracle.com>> wrote:

  Hi Guys,


  Could someone please review my fix to add missing standard
  constructor
  overloads to NoSuchElementException class and update the AbstractList
  class to use them.


  A CSR was filed and approved. Along with the code change a new
  test is
  added to verify the behavior.


  Please find the webrev at  -


  http://cr.openjdk.java.net/~kravikumar/8161558/webrev.00/


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


  Thanks,

  Kiran

Re: [15] RFR (xs) 8046362 IdentityHashMap.hash comments should be clarified

[Stuart Marks]

Hi Roger,

I don't want to have a detailed discussion of the performance tradeoffs in the 
comments. My comments changes are intended to address two specific points: the 
"multiply by -127" phrase and the arrangement of alternating keys and values in 
the array. Both have been sources of confusion in the past and have even 
resulted in bug reports.


I'll change "JRE" to "Java" per your suggestion.

s'marks


On 2/11/20 6:41 AM, Roger Riggs wrote:

Hi Stuart,

The implNote compares the performance impact without mentioning
that HashMap uses 'equals()' vs '==' which might have as big an impact
on performance as sequential vs chained references.
Detailed performance comparisons are very difficult given the factors
that impact performance, cache sizes, data structure sizes, etc.
I'm not sure the wordsmithing adds much to the doc.

"JRE" -> "Java"

$.02, Roger

On 2/10/20 7:09 PM, Stuart Marks wrote:



On 2/10/20 7:52 AM, Martin Buchholz wrote:

On Fri, Feb 7, 2020 at 2:47 PM David Holmes  wrote:


Hi Martin,

On 8/02/2020 3:10 am, Martin Buchholz wrote:

I explored System.identityHashCode (see below).


System.identityHashCode is a VM call that is potentially very expensive.
In the worst-case it triggers monitor inflation.



I can believe that.  The VM can't simply return the address of an object
because ... the object might move and an address is a poor hash code
because low bits will tend to be zero.

But  IdentityHashMap already calls System.identityHashCode - that price
must be paid.

My proposal is still "Let's trust System.identityHashCode to produce
acceptable hash codes"


OK... I appreciate that there are a bunch of subtle issues here, from whether 
it's necessary to do mixing of the identity hash code to whether an 
open-addressed table really is faster than separate chaining in this case. 
Before we go on too long, though, I'd like to ask what to do about my proposed 
comments changes.


Should I proceed with pushing my comments changes (which I've appended below 
for convenience)? Or is somebody going to dive in right away and make changes 
that will invalidate my proposed comments changes, and thus I should hold off 
pushing them?


Thanks,

s'marks





# HG changeset patch
# User smarks
# Date 1580167577 28800
#  Mon Jan 27 15:26:17 2020 -0800
# Node ID 0e08ce23484ca42597105225ffa3dd0827cb4b60
# Parent  981f6982717a7df4a2a43dd0ae6b2c2389e683f9
8046362: IdentityHashMap.hash comments should be clarified
Reviewed-by: XXX

diff -r 981f6982717a -r 0e08ce23484c 
src/java.base/share/classes/java/util/IdentityHashMap.java
--- a/src/java.base/share/classes/java/util/IdentityHashMap.java Mon Jan 27 
14:03:58 2020 -0800
+++ b/src/java.base/share/classes/java/util/IdentityHashMap.java Mon Jan 27 
15:26:17 2020 -0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2019, 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
@@ -115,17 +115,19 @@
  * exception for its correctness: fail-fast iterators should be used only
  * to detect bugs.
  *
- * Implementation note: This is a simple linear-probe hash table,
- * as described for example in texts by Sedgewick and Knuth.  The array
- * alternates holding keys and values.  (This has better locality for large
- * tables than does using separate arrays.)  For many JRE implementations
- * and operation mixes, this class will yield better performance than
- * {@link HashMap} (which uses chaining rather than linear-probing).
- *
  * This class is a member of the
  * href="{@docRoot}/java.base/java/util/package-summary.html#CollectionsFramework">

  * Java Collections Framework.
  *
+ * @implNote
+ * This is a simple linear-probe hash table,
+ * as described for example in texts by Sedgewick and Knuth.  The array
+ * contains alternating keys and values, with keys at even indexes and values
+ * at odd indexes. (This arrangement has better locality for large
+ * tables than does using separate arrays.)  For many JRE implementations
+ * and operation mixes, this class will yield better performance than
+ * {@link HashMap}, which uses chaining rather than linear-probing.
+ *
  * @see System#identityHashCode(Object)
  * @see Object#hashCode()
  * @see Collection
@@ -293,7 +295,7 @@
  */
 private static int hash(Object x, int length) {
 int h = System.identityHashCode(x);
-    // Multiply by -127, and left-shift to use least bit as part of hash
+    // Multiply by -254 to use the hash LSB and to ensure index is even
 return ((h << 1) - (h << 8)) & (length - 1);
 }

Re: [15] RFR (xs) 8046362 IdentityHashMap.hash comments should be clarified

[Stuart Marks]

On 2/10/20 7:52 AM, Martin Buchholz wrote:

On Fri, Feb 7, 2020 at 2:47 PM David Holmes  wrote:


Hi Martin,

On 8/02/2020 3:10 am, Martin Buchholz wrote:

I explored System.identityHashCode (see below).


System.identityHashCode is a VM call that is potentially very expensive.
In the worst-case it triggers monitor inflation.



I can believe that.  The VM can't simply return the address of an object
because ... the object might move and an address is a poor hash code
because low bits will tend to be zero.

But  IdentityHashMap already calls System.identityHashCode - that price
must be paid.

My proposal is still "Let's trust System.identityHashCode to produce
acceptable hash codes"


OK... I appreciate that there are a bunch of subtle issues here, from whether 
it's necessary to do mixing of the identity hash code to whether an 
open-addressed table really is faster than separate chaining in this case. 
Before we go on too long, though, I'd like to ask what to do about my proposed 
comments changes.


Should I proceed with pushing my comments changes (which I've appended below for 
convenience)? Or is somebody going to dive in right away and make changes that 
will invalidate my proposed comments changes, and thus I should hold off pushing 
them?


Thanks,

s'marks





# HG changeset patch
# User smarks
# Date 1580167577 28800
#  Mon Jan 27 15:26:17 2020 -0800
# Node ID 0e08ce23484ca42597105225ffa3dd0827cb4b60
# Parent  981f6982717a7df4a2a43dd0ae6b2c2389e683f9
8046362: IdentityHashMap.hash comments should be clarified
Reviewed-by: XXX

diff -r 981f6982717a -r 0e08ce23484c 
src/java.base/share/classes/java/util/IdentityHashMap.java
--- a/src/java.base/share/classes/java/util/IdentityHashMap.javaMon Jan 27 
14:03:58 2020 -0800
+++ b/src/java.base/share/classes/java/util/IdentityHashMap.javaMon Jan 27 
15:26:17 2020 -0800

@@ -1,5 +1,5 @@
 /*
- * Copyright (c) 2000, 2019, 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
@@ -115,17 +115,19 @@
  * exception for its correctness: fail-fast iterators should be used only
  * to detect bugs.
  *
- * Implementation note: This is a simple linear-probe hash table,
- * as described for example in texts by Sedgewick and Knuth.  The array
- * alternates holding keys and values.  (This has better locality for large
- * tables than does using separate arrays.)  For many JRE implementations
- * and operation mixes, this class will yield better performance than
- * {@link HashMap} (which uses chaining rather than linear-probing).
- *
  * This class is a member of the
  * href="{@docRoot}/java.base/java/util/package-summary.html#CollectionsFramework">

  * Java Collections Framework.
  *
+ * @implNote
+ * This is a simple linear-probe hash table,
+ * as described for example in texts by Sedgewick and Knuth.  The array
+ * contains alternating keys and values, with keys at even indexes and values
+ * at odd indexes. (This arrangement has better locality for large
+ * tables than does using separate arrays.)  For many JRE implementations
+ * and operation mixes, this class will yield better performance than
+ * {@link HashMap}, which uses chaining rather than linear-probing.
+ *
  * @see System#identityHashCode(Object)
  * @see Object#hashCode()
  * @see Collection
@@ -293,7 +295,7 @@
  */
 private static int hash(Object x, int length) {
 int h = System.identityHashCode(x);
-// Multiply by -127, and left-shift to use least bit as part of hash
+// Multiply by -254 to use the hash LSB and to ensure index is even
 return ((h << 1) - (h << 8)) & (length - 1);
 }

Re: RFR [15] 8161558: ListIterator should not discard cause on exception

[Stuart Marks]

Hi Kiran,

Thanks for reworking the test. This looks good to me. One small thing:

  53 throw new IndexOutOfBoundsException();

This should include the offending index in the IOOBE. This might make Rémi 
happy. (Then again, it might not.) :-)


I think this covers the concerns that Martin has, but let's wait a bit to hear 
from him first.


s'marks

On 2/5/20 11:49 AM, Kiran Ravikumar wrote:

Thanks Stuart and Martin,


Here is an updated webrev with the changes.

http://cr.openjdk.java.net/~kravikumar/8161558/webrev.01/

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


Thanks,

Kiran


On 15/01/2020 12:46, Martin Buchholz wrote:

Hi Kiran,

Looks good to me, but I always nitpick ...

Stray semicolon?
       var iterator = list.listIterator(list.size());; // position at end

I would have documented whitebox test assumptions: that nCopies iterators are 
implemented via AbstractList, and that AbstractList's list iterator inherits 
behavior from iterator.


I probably would have added a plain iterator test, and might have refactored 
the code that tests the exception.



On Wed, Jan 15, 2020 at 4:07 AM Kiran Ravikumar 
> wrote:


Hi Guys,


Could someone please review my fix to add missing standard constructor
overloads to NoSuchElementException class and update the AbstractList
class to use them.


A CSR was filed and approved. Along with the code change a new test is
added to verify the behavior.


Please find the webrev at  -


http://cr.openjdk.java.net/~kravikumar/8161558/webrev.00/


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


Thanks,

Kiran

Re: RFR [15] 8161558: ListIterator should not discard cause on exception

[Stuart Marks]

Hi Remi,

The bug report originally requested that a bunch of different exceptions include 
a cause. I don't think the cause should be added to all of them. The cases that 
Kiran is adding are ones where I thought that adding a cause does have value.


If someone is using a ListIterator (or a plain Iterator for that matter) they 
might have an incorrect model for what the index value is after a certain 
operation (remove, for example), and they might get an NSEE unexpectedly. They 
might reasonably wonder what the state of the iterator is that resulted in that 
exception. Without a cause, NSEE doesn't have that information. Chaining the 
IOOBE will usually include the index that caused the problem, which I think is 
useful in such circumstances.


The iterators in AbstractList all keep track of indexes and call get() for 
access to the appropriate element. I don't think they should do a bounds check 
and throw an exception on that basis, because the bounds could change between 
the time they're checked and the call to get(). Thus, the iterators would have 
to catch the exception from get() even if the bounds are checked in advance, 
making the bounds check redundant.


s'marks

On 2/5/20 2:05 PM, Remi Forax wrote:

Stuart, Martin, Kiran,
I think this "bug" should not be fixed because it's one of the cases where 
providing more information is actually bad from a user POV.

The current code throws NoSuchElementException when the iterator reach the end 
so from the user POV, this is the right exception because of the right issue, 
so from the user POV there is no need to change the actual code.
If we chain the exception,
- it's less clear from a user POV
- the user may think that there is an error in the AbstractList implementation 
but it's not the case, it's just that AbstractList iterators next method is 
implemented weirdly, it prefers to go out of bound instead of checking the 
bound.

I'm okay with NoSuchElementException having a new constructor that takes a 
chained exception but i really think that in this specific case, it's a bad 
idea(TM) to use it to propagate an exception to the user that it should not 
care about.

BTW, perhaps, those method next() should be re-written to test the bound instead of 
catching the IOOBE because i'm not sure this "optimization" make sense nowadays.

regards,
Rémi

- Mail original -

De: "Kiran Ravikumar" 
À: "core-libs-dev" 
Envoyé: Mercredi 5 Février 2020 20:49:09
Objet: Re: RFR [15] 8161558: ListIterator should not discard cause on exception



Thanks Stuart and Martin,


Here is an updated webrev with the changes.

http://cr.openjdk.java.net/~kravikumar/8161558/webrev.01/

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


Thanks,

Kiran


On 15/01/2020 12:46, Martin Buchholz wrote:

Hi Kiran,

Looks good to me, but I always nitpick ...

Stray semicolon?
        var iterator = list.listIterator(list.size());; // position at end

I would have documented whitebox test assumptions: that nCopies
iterators are implemented via AbstractList, and that
AbstractList's list iterator inherits behavior from iterator.

I probably would have added a plain iterator test, and might have
refactored the code that tests the exception.


On Wed, Jan 15, 2020 at 4:07 AM Kiran Ravikumar
mailto:kiran.sidhartha.raviku...@oracle.com>> wrote:

 Hi Guys,


 Could someone please review my fix to add missing standard
 constructor
 overloads to NoSuchElementException class and update the AbstractList
 class to use them.


 A CSR was filed and approved. Along with the code change a new
 test is
 added to verify the behavior.


 Please find the webrev at  -


 http://cr.openjdk.java.net/~kravikumar/8161558/webrev.00/


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


 Thanks,

 Kiran