8199318: add idempotent copy operation for Map.Entry

[stuart-marks]

I'm fixing this along with a couple intertwined issues.

1. Add Map.Entry::copyOf as an idempotent copy operation.

2. Clarify that AbstractMap.SimpleImmutableEntry is itself unmodifiable (not really immutable) but that subclasses can be modifiable.

3. Clarify some confusing, historical wording about Map.Entry instances being obtainable only via iteration of a Map's entry-set view. This was never really true, since anyone could implement the Map.Entry interface, and it certainly was not true since JDK 1.6 when SimpleEntry and SimpleImmutableEntry were added.

---

Progress

• Change must not contain extraneous whitespace
• Commit message must refer to an issue
• Change must be properly reviewed

Issue

• JDK-8199318: add idempotent copy operation for Map.Entry

Reviewers

• Alan Bateman (@AlanBateman - Reviewer) ⚠️ Review applies to 841a154
• Paul Sandoz (@PaulSandoz - Reviewer)
• Daniel Fuchs (@dfuch - Reviewer)

Reviewing

Using git

Checkout this PR locally:
$ git fetch https://git.openjdk.java.net/jdk pull/4295/head:pull/4295
$ git checkout pull/4295

Update a local copy of the PR:
$ git checkout pull/4295
$ git pull https://git.openjdk.java.net/jdk pull/4295/head

Using Skara CLI tools

Checkout this PR locally:
$ git pr checkout 4295

View PR using the GUI difftool:
$ git pr show -t 4295

Using diff file

Download this PR as a diff file:
https://git.openjdk.java.net/jdk/pull/4295.diff

[bridgekeeper]

👋 Welcome back smarks! A progress list of the required criteria for merging this PR into master will be added to the body of your pull request. There are additional pull request commands available for use with this pull request.

[openjdk]

@stuart-marks The following label will be automatically applied to this pull request:

• core-libs

When this pull request is ready to be reviewed, an "RFR" email will be sent to the corresponding mailing list. If you would like to change these labels, use the /label pull request command.

[stuart-marks]

/csr

[openjdk]

@stuart-marks has indicated that a compatibility and specification (CSR) request is needed for this pull request.
@stuart-marks please create a CSR request for issue JDK-8199318. This pull request cannot be integrated until the CSR request is approved.

[mlbridge]

Webrevs

• 01: Full - Incremental (c67b6445)
• 00: Full (841a154c)

[forax]

LGTM,
i wonder if we should not declare SimpleImmutableEntry final, while it's not a backward compatible change,
it's may be better on the long term because SimpleImmutableEntry is already used as an immutable type,
so instead of documenting the fact that SimpleImmutableEntry is not declared final thus SimpleImmutableEntry as a type does not guarantte shallow immutability, it may be better to fix the root cause.

[AlanBateman]

I read through the javadoc and API notes and it all looks reasonable.

[stuart-marks]

i wonder if we should not declare SimpleImmutableEntry final, while it's not a backward compatible change,
it's may be better on the long term because SimpleImmutableEntry is already used as an immutable type,
so instead of documenting the fact that SimpleImmutableEntry is not declared final thus SimpleImmutableEntry as a type does not guarantte shallow immutability, it may be better to fix the root cause.

A quick search reveals that Guava has a public subclass of SimpleImmutableEntry:

https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/cache/RemovalNotification.html

There are possibly others. It doesn't seem worth the incompatibility to me, as it would break stuff in order to have only a "cleaner" meaning for "immutable." Also, there are other classes in the JDK that claim they are immutable but which can be subclassed, e.g., BigInteger. I don't see a good way of fixing this.

[forax]

A quick search reveals that Guava has a public subclass of SimpleImmutableEntry:
https://guava.dev/releases/30.1.1-jre/api/docs/com/google/common/cache/RemovalNotification.html

There are possibly others. It doesn't seem worth the incompatibility to me, as it would break stuff in order to have only a "cleaner" meaning for "immutable." Also, there are other classes in the JDK that claim they are immutable but which can be subclassed, e.g., BigInteger. I don't see a good way of fixing this.

Thanks, i've done some digging too,
I foolishly hoped that nobody would subclass a class with Immutable in its name,
oh boy i was wrong :)

So documenting the existing behavior seems the only possible way.

[dfuch]

In that case then maybe it should be {@code Entry} in both places where entry was replaced with Entry ?

[stuart-marks]

Maybe. We're not terribly consistent about this. A fair amount of the docs just uses a plain-text, capitalized form of a class name, as opposed to the code font. Using the code font everywhere adds clutter to both the markup and to the rendered output. In this case I wanted to distinguish the generic mention of an entry in a map from an instance of an Entry object. In cases where it needs to be absolutely clear, I used Map.Entry (the qualified name, in code font). Doing that here seems like it would add too much clutter though.

[mlbridge]

Mailing list message from Peter Levart on core-libs-dev:

On 02/06/2021 19:24, R?mi Forax wrote:

I foolishly hoped that nobody would subclass a class with `Immutable` in its name,
oh boy i was wrong:)

There's nothing wrong in subclassing an immutable class. As long as the
subclass keeps being immutable. Was it subclassed and made mutable by that?

Peter

[openjdk]

@stuart-marks This change now passes all automated pre-integration checks.

ℹ️ This project also has non-automated pre-integration requirements. Please see the file CONTRIBUTING.md for details.

After integration, the commit message for the final commit will be:

8199318: add idempotent copy operation for Map.Entry

Reviewed-by: alanb, psandoz, dfuchs

You can use pull request commands such as /summary, /contributor and /issue to adjust it as needed.

At the time when this comment was updated there had been 96 new commits pushed to the master branch:

• af3df63: 8267598: jpackage removes system libraries from java.library.path
• 52d8215: 8268131: 2 java/foreign tests timed out
• 5405f98: 8268077: java.util.List missing from Collections Framework Overview
• 3aa7062: 8262409: sun/security/ssl/SSLSocketImpl/SSLSocketImplThrowsWrongExceptions. SSL test failures caused by java failed with "Server reported the wrong exception"
• 5982cfc: 8266317: Vector API enhancements
• eb385c0: 8268167: MultipleLogins.java failure on macosx-aarch64
• fbaebd4: 8268014: Build failure on SUSE Linux Enterprise Server 11.4 (s390x) due to 'SYS_get_mempolicy' was not declared
• 338dae4: 8268133: Update java/net/Authenticator tests to eliminate dependency on sun.net.www.MessageHeader and some other internal APIs
• 29ab162: 8266257: Fix foreign linker build issues for ppc and s390
• c8f4c02: 8268118: Rename bytes_os_cpu.inline.hpp files to bytes_os_cpu.hpp
• ... and 86 more: https://git.openjdk.java.net/jdk/compare/95b1fa7a88ec3c017734c9d0a6b6b6117f74a610...master

As there are no conflicts, your changes will automatically be rebased on top of these commits when integrating. If you prefer to avoid this automatic rebasing, please check the documentation for the /integrate command for further details.

➡️ To integrate this PR with the above commit message to the master branch, type /integrate in a new comment.

[mlbridge]

Mailing list message from Remi Forax on core-libs-dev:

----- Mail original -----

De: "Peter Levart" <peter.levart at gmail.com>
?: "R?mi Forax" <forax at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Envoy?: Jeudi 3 Juin 2021 20:49:05
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

On 02/06/2021 19:24, R?mi Forax wrote:

I foolishly hoped that nobody would subclass a class with `Immutable` in its
name,
oh boy i was wrong:)

There's nothing wrong in subclassing an immutable class. As long as the
subclass keeps being immutable. Was it subclassed and made mutable by that?

It has to be immutable all the way up, you have the same issue if the subclass is not final.

If you filter out guava an google cache, github get you 12 pages of result and only one stupid code
https://github.com/klayders/interlude/blob/95fd59a911d2baa8ac36ae6b877955aa4fbd095e/src/main/java/l2/gameserver/skills/SkillEntry.java#L12

A lot of code uses SimpleImmutableEntry as a pair, my hope is that the introduction of records will clean that practice.

That said, there is also several broken codes, mostly two issues,
either a.equals(n) is not equivalent to b.equals(a) or a.equals(b) is not equivalent to a.compareTo(b) == 0.

I kind of regret that the compiler does not provide automatically an implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to find/reproduce.

Peter

R?mi

[mlbridge]

Mailing list message from Remi Forax on core-libs-dev:

----- Mail original -----

De: "Peter Levart" <peter.levart at gmail.com>
?: "R?mi Forax" <forax at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Envoy?: Jeudi 3 Juin 2021 20:49:05
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

On 02/06/2021 19:24, R?mi Forax wrote:

I foolishly hoped that nobody would subclass a class with `Immutable` in its
name,
oh boy i was wrong:)

There's nothing wrong in subclassing an immutable class. As long as the
subclass keeps being immutable. Was it subclassed and made mutable by that?

It has to be immutable all the way up, you have the same issue if the subclass is not final.

If you filter out guava an google cache, github get you 12 pages of result and only one stupid code
https://github.com/klayders/interlude/blob/95fd59a911d2baa8ac36ae6b877955aa4fbd095e/src/main/java/l2/gameserver/skills/SkillEntry.java#L12

A lot of code uses SimpleImmutableEntry as a pair, my hope is that the introduction of records will clean that practice.

That said, there is also several broken codes, mostly two issues,
either a.equals(n) is not equivalent to b.equals(a) or a.equals(b) is not equivalent to a.compareTo(b) == 0.

I kind of regret that the compiler does not provide automatically an implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to find/reproduce.

Peter

R?mi

[mlbridge]

Mailing list message from John Rose on core-libs-dev:

On Jun 3, 2021, at 12:46 PM, Remi Forax <forax at univ-mlv.fr<mailto:forax at univ-mlv.fr>> wrote:

I kind of regret that the compiler does not provide automatically an implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to find/reproduce.

That?s a slippery slope. IIRC we consciously stopped
before that step.

That said, there are other ways to fix this. We should
have utilities (maybe in the JDK but not the JLS) which
build such methods and make it easy for users to grab onto
them. Maybe something like this:

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java

? John

[mlbridge]

Mailing list message from John Rose on core-libs-dev:

On Jun 3, 2021, at 12:46 PM, Remi Forax <forax at univ-mlv.fr<mailto:forax at univ-mlv.fr>> wrote:

I kind of regret that the compiler does not provide automatically an implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to find/reproduce.

That?s a slippery slope. IIRC we consciously stopped
before that step.

That said, there are other ways to fix this. We should
have utilities (maybe in the JDK but not the JLS) which
build such methods and make it easy for users to grab onto
them. Maybe something like this:

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java

? John

[mlbridge]

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "John Rose" <john.r.rose at oracle.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Peter Levart" <peter.levart at gmail.com>, "R?mi Forax"
<forax at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

On Jun 3, 2021, at 12:46 PM, Remi Forax < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] > wrote:

I kind of regret that the compiler does not provide automatically an
implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to
find/reproduce.

That?s a slippery slope. IIRC we consciously stopped
before that step.

That said, there are other ways to fix this. We should
have utilities (maybe in the JDK but not the JLS) which
build such methods and make it easy for users to grab onto
them. Maybe something like this:

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

[ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

The main issue with this kind of code is that the JIT does not see through the ClassValue.

Tweaking a little bit your code, I get
(It's a PITA that we have to use a raw type to workaround circularly defined parameter type)

import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.stream.Stream;

@SuppressWarnings({"rawtypes","unchecked"})
interface ComparableRecord<T extends Record & ComparableRecord<T>> extends Comparable<T> {
@Override default int compareTo(T that) {
if (this.getClass() != that.getClass()) {
throw new IllegalArgumentException("not same class");
}
return COMPARE_TO_METHODS.get(this.getClass()).compare(this, that);
}
static final ClassValue<Comparator<Object>> COMPARE_TO_METHODS = new ClassValue<>() {
@Override
protected Comparator<Object> computeValue(Class<?> type) {
return Stream.of(type.getRecordComponents())
.map(component -> {
var accessor = component.getAccessor();
return Comparator.<Object, Comparable>comparing(r -> {
try {
return (Comparable<?>) accessor.invoke(r);
} catch (ReflectiveOperationException ex) {
throw new IllegalArgumentException(ex);
}
});
})
.reduce((r1, r2) -> 0, Comparator::thenComparing, (_1, _2) -> { throw null; });
}
};

static void main(String[] args) {
record Foo(int x, String y) implements ComparableRecord<Foo> { }

var list = Stream.of(new Foo(2, "foo"), new Foo(2, "bar")) .sorted().toList();
System.out.println(list);
}
}

? John

[mlbridge]

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "John Rose" <john.r.rose at oracle.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Peter Levart" <peter.levart at gmail.com>, "R?mi Forax"
<forax at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

On Jun 3, 2021, at 12:46 PM, Remi Forax < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] > wrote:

I kind of regret that the compiler does not provide automatically an
implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to
find/reproduce.

That?s a slippery slope. IIRC we consciously stopped
before that step.

That said, there are other ways to fix this. We should
have utilities (maybe in the JDK but not the JLS) which
build such methods and make it easy for users to grab onto
them. Maybe something like this:

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

[ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

The main issue with this kind of code is that the JIT does not see through the ClassValue.

Tweaking a little bit your code, I get
(It's a PITA that we have to use a raw type to workaround circularly defined parameter type)

import java.util.ArrayList;
import java.util.Comparator;
import java.util.List;
import java.util.stream.Stream;

@SuppressWarnings({"rawtypes","unchecked"})
interface ComparableRecord<T extends Record & ComparableRecord<T>> extends Comparable<T> {
@Override default int compareTo(T that) {
if (this.getClass() != that.getClass()) {
throw new IllegalArgumentException("not same class");
}
return COMPARE_TO_METHODS.get(this.getClass()).compare(this, that);
}
static final ClassValue<Comparator<Object>> COMPARE_TO_METHODS = new ClassValue<>() {
@Override
protected Comparator<Object> computeValue(Class<?> type) {
return Stream.of(type.getRecordComponents())
.map(component -> {
var accessor = component.getAccessor();
return Comparator.<Object, Comparable>comparing(r -> {
try {
return (Comparable<?>) accessor.invoke(r);
} catch (ReflectiveOperationException ex) {
throw new IllegalArgumentException(ex);
}
});
})
.reduce((r1, r2) -> 0, Comparator::thenComparing, (_1, _2) -> { throw null; });
}
};

static void main(String[] args) {
record Foo(int x, String y) implements ComparableRecord<Foo> { }

var list = Stream.of(new Foo(2, "foo"), new Foo(2, "bar")) .sorted().toList();
System.out.println(list);
}
}

? John

[mlbridge]

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "John Rose" <john. r.rose at oracle.com >
?: "Remi Forax" < forax at univ-mlv.fr >
Cc: "Peter Levart" < peter.levart at gmail.com >, "R?mi Forax" <
forax at openjdk.java.net >, "core-libs-dev" < core-libs-dev at openjdk.java.net >
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

On Jun 3, 2021, at 12:46 PM, Remi Forax < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] > wrote:

I kind of regret that the compiler does not provide automatically an
implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to
find/reproduce.

That?s a slippery slope. IIRC we consciously stopped
before that step.

That said, there are other ways to fix this. We should
have utilities (maybe in the JDK but not the JLS) which
build such methods and make it easy for users to grab onto
them. Maybe something like this:

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

[ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

[repost with a link]

The main issue with this kind of code is that the JIT does not see through the ClassValue.

Tweaking a little bit your code, I get
https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8

(It's a PITA that we have to use a raw type to workaround circularly defined parameter type)

? John

R?mi

[mlbridge]

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "John Rose" <john. r.rose at oracle.com >
?: "Remi Forax" < forax at univ-mlv.fr >
Cc: "Peter Levart" < peter.levart at gmail.com >, "R?mi Forax" <
forax at openjdk.java.net >, "core-libs-dev" < core-libs-dev at openjdk.java.net >
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

On Jun 3, 2021, at 12:46 PM, Remi Forax < [ mailto:forax at univ-mlv.fr |
forax at univ-mlv.fr ] > wrote:

I kind of regret that the compiler does not provide automatically an
implementation of compareTo if the record implements Comparable.
People sucks at writing compareTo and the resulting bugs are hard to
find/reproduce.

That?s a slippery slope. IIRC we consciously stopped
before that step.

That said, there are other ways to fix this. We should
have utilities (maybe in the JDK but not the JLS) which
build such methods and make it easy for users to grab onto
them. Maybe something like this:

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

[ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

[repost with a link]

The main issue with this kind of code is that the JIT does not see through the ClassValue.

Tweaking a little bit your code, I get
https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8

(It's a PITA that we have to use a raw type to workaround circularly defined parameter type)

? John

R?mi

[mlbridge]

Mailing list message from John Rose on core-libs-dev:

On Jun 3, 2021, at 3:17 PM, forax at univ-mlv.fr<mailto:forax at univ-mlv.fr> wrote:

________________________________
De: "John Rose" <john.r.rose at oracle.com<mailto:r.rose at oracle.com>>
?: "Remi Forax" <forax at univ-mlv.fr<mailto:forax at univ-mlv.fr>>
Cc: "Peter Levart" <peter.levart at gmail.com<mailto:peter.levart at gmail.com>>, "R?mi Forax" <forax at openjdk.java.net<mailto:forax at openjdk.java.net>>, "core-libs-dev" <core-libs-dev at openjdk.java.net<mailto:core-libs-dev at openjdk.java.net>>
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry
...

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java

[repost with a link]

The main issue with this kind of code is that the JIT does not see through the ClassValue.

Wow, it?s almost as if we?ve discussed this already! ;-)
So, https://bugs.openjdk.java.net/browse/JDK-8238260
Part of my agenda here is finding more reasons why
JDK-8238260 deserves some love.

Currently, the translation strategy for Records
requires a lot of boilerplate generated into each
subclass for toString, equals, and hashCode.
It is partially declarative, because it uses indy.
So, yay for that. But it is also a bad smell (IMO)
that the trick needs to be played in each subclass.

If ClassValue were performant, we could have
just one method in Record for each of toString,
equals, and hashCode, and have them DTRT.
The user code would then be able to call
super.toString() to get the standard behavior
in a refining wrapper method in a subclass.

Looking further, it would be nice to have methods
which (a) inherit from supers as reusable code
ought to, but (b) re-specialize themselves once
per subclass indy-style. This is a VM trick I hope
to look into after we do specialization. For now,
ClassValue is the way to simulate that.

Tweaking a little bit your code, I get
https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8<https://urldefense.com/v3/__https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8__;!!GqivPVa7Brio!MheW9rHDHXlXYNKUju7v5G0vXlpj1YOoDWFjG9AcpnXnVz2TxlMYDM7i86yFtT_B$>

(It's a PITA that we have to use a raw type to workaround circularly defined parameter type)

I tried to DTRT and got into Inference Hell, surrounded
by a swarms of unfriendly wildcards with pitchforks.
Glad it wasn?t just me.

Inspired by your more whole-hearted use of streams
to build the code, I updated my example. Thanks.

? John

[mlbridge]

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "John Rose" <john.r.rose at oracle.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Peter Levart" <peter.levart at gmail.com>, "R?mi Forax"
<forax at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Envoy?: Vendredi 4 Juin 2021 02:05:41
Objet: Re: [External] : Re: RFR: 8199318: add idempotent copy operation for
Map.Entry

On Jun 3, 2021, at 3:17 PM, [ mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ]
wrote:

De: "John Rose" <john. [ mailto:r.rose at oracle.com | r.rose at oracle.com ] >
?: "Remi Forax" < [ mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >
Cc: "Peter Levart" < [ mailto:peter.levart at gmail.com | peter.levart at gmail.com ]

, "R?mi Forax" < [ mailto:forax at openjdk.java.net | forax at openjdk.java.net ] >,
"core-libs-dev" < [ mailto:core-libs-dev at openjdk.java.net |
core-libs-dev at openjdk.java.net ] >
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

...

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

[ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

[repost with a link]

The main issue with this kind of code is that the JIT does not see through the
ClassValue.

Wow, it?s almost as if we?ve discussed this already! ;-)
So, [ https://bugs.openjdk.java.net/browse/JDK-8238260 |
https://bugs.openjdk.java.net/browse/JDK-8238260 ]
Part of my agenda here is finding more reasons why
JDK-8238260 deserves some love.

Currently, the translation strategy for Records
requires a lot of boilerplate generated into each
subclass for toString, equals, and hashCode.
It is partially declarative, because it uses indy.
So, yay for that. But it is also a bad smell (IMO)
that the trick needs to be played in each subclass.

If ClassValue were performant, we could have
just one method in Record for each of toString,
equals, and hashCode, and have them DTRT.
The user code would then be able to call
super.toString() to get the standard behavior
in a refining wrapper method in a subclass.

Looking further, it would be nice to have methods
which (a) inherit from supers as reusable code
ought to, but (b) re-specialize themselves once
per subclass indy-style. This is a VM trick I hope
to look into after we do specialization. For now,
ClassValue is the way to simulate that.

yes, it's a specialization problem.
I wonder if it an be resolved using a combination of a species-static variable and magic shibboleth to ask the type parameter to be always reified

interface ComparableRecord <please-always-reified-me T extends Record & ComparableRecord< T > > extends Comparable< T > {
species-static Comparator<T> COMPARATOR; // a parameteric condy ?
species-static {
COMPARATOR = ...
}

default int compareTo(T other) {
return COMPARATOR.compare(this, other);
}
}

Tweaking a little bit your code, I get
[
https://urldefense.com/v3/__https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8__;!!GqivPVa7Brio!MheW9rHDHXlXYNKUju7v5G0vXlpj1YOoDWFjG9AcpnXnVz2TxlMYDM7i86yFtT_B$
| https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8 ]

(It's a PITA that we have to use a raw type to workaround circularly defined
parameter type)

I tried to DTRT and got into Inference Hell, surrounded
by a swarms of unfriendly wildcards with pitchforks.
Glad it wasn?t just me.

Inspired by your more whole-hearted use of streams
to build the code, I updated my example. Thanks.

? John

R?mi

[mlbridge]

Mailing list message from forax at univ-mlv.fr on core-libs-dev:

De: "John Rose" <john.r.rose at oracle.com>
?: "Remi Forax" <forax at univ-mlv.fr>
Cc: "Peter Levart" <peter.levart at gmail.com>, "R?mi Forax"
<forax at openjdk.java.net>, "core-libs-dev" <core-libs-dev at openjdk.java.net>
Envoy?: Vendredi 4 Juin 2021 02:05:41
Objet: Re: [External] : Re: RFR: 8199318: add idempotent copy operation for
Map.Entry

On Jun 3, 2021, at 3:17 PM, [ mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ]
wrote:

De: "John Rose" <john. [ mailto:r.rose at oracle.com | r.rose at oracle.com ] >
?: "Remi Forax" < [ mailto:forax at univ-mlv.fr | forax at univ-mlv.fr ] >
Cc: "Peter Levart" < [ mailto:peter.levart at gmail.com | peter.levart at gmail.com ]

, "R?mi Forax" < [ mailto:forax at openjdk.java.net | forax at openjdk.java.net ] >,
"core-libs-dev" < [ mailto:core-libs-dev at openjdk.java.net |
core-libs-dev at openjdk.java.net ] >
Envoy?: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry

...

interface ComparableRecord<T extends Record & ComparableRecord<T>>
extends Comparable<T> { ? }

record Foo(int x, String y) implements ComparableRecord<Foo> { ? }

[ http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java |
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java ]

[repost with a link]

The main issue with this kind of code is that the JIT does not see through the
ClassValue.

Wow, it?s almost as if we?ve discussed this already! ;-)
So, [ https://bugs.openjdk.java.net/browse/JDK-8238260 |
https://bugs.openjdk.java.net/browse/JDK-8238260 ]
Part of my agenda here is finding more reasons why
JDK-8238260 deserves some love.

Currently, the translation strategy for Records
requires a lot of boilerplate generated into each
subclass for toString, equals, and hashCode.
It is partially declarative, because it uses indy.
So, yay for that. But it is also a bad smell (IMO)
that the trick needs to be played in each subclass.

If ClassValue were performant, we could have
just one method in Record for each of toString,
equals, and hashCode, and have them DTRT.
The user code would then be able to call
super.toString() to get the standard behavior
in a refining wrapper method in a subclass.

Looking further, it would be nice to have methods
which (a) inherit from supers as reusable code
ought to, but (b) re-specialize themselves once
per subclass indy-style. This is a VM trick I hope
to look into after we do specialization. For now,
ClassValue is the way to simulate that.

yes, it's a specialization problem.
I wonder if it an be resolved using a combination of a species-static variable and magic shibboleth to ask the type parameter to be always reified

interface ComparableRecord <please-always-reified-me T extends Record & ComparableRecord< T > > extends Comparable< T > {
species-static Comparator<T> COMPARATOR; // a parameteric condy ?
species-static {
COMPARATOR = ...
}

default int compareTo(T other) {
return COMPARATOR.compare(this, other);
}
}

Tweaking a little bit your code, I get
[
https://urldefense.com/v3/__https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8__;!!GqivPVa7Brio!MheW9rHDHXlXYNKUju7v5G0vXlpj1YOoDWFjG9AcpnXnVz2TxlMYDM7i86yFtT_B$
| https://gist.github.com/forax/e76367e1a90bf011692ee9bec65ff0f8 ]

(It's a PITA that we have to use a raw type to workaround circularly defined
parameter type)

I tried to DTRT and got into Inference Hell, surrounded
by a swarms of unfriendly wildcards with pitchforks.
Glad it wasn?t just me.

Inspired by your more whole-hearted use of streams
to build the code, I updated my example. Thanks.

? John

R?mi

[stuart-marks]

/integrate

[openjdk]

@stuart-marks Since your change was applied there have been 112 commits pushed to the master branch:

• b27599b: 8268222: javax/xml/jaxp/unittest/transform/Bug6216226Test.java failed, cannot delete file
• 59a539f: 8268129: LibraryLookup::ofDefault leaks symbols from loaded libraries
• 40c9e25: 8265444: Javadocs: jdk.jshell - small typo
• 069f180: 8268174: Move x86-specific stub declarations into stubRoutines_x86.hpp
• 3025f05: 8264305: Create implementation for native accessibility peer for Statusbar java role
• e2d5ff9: 8268214: Use system zlib and disable dtrace when building linux-aarch64 at Oracle
• 1b4378e: 8268142: Switch to jdk-17+24 for macosx-aarch64 at Oracle
• edca245: 8267917: mark hotspot containers tests which ignore external VM flags
• 05df172: 8268224: Cleanup references to "strictfp" in core lib comments
• 516e60a: 8268095: CDS MethodHandle tests should add -XX:-VerifyDependencies
• ... and 102 more: https://git.openjdk.java.net/jdk/compare/95b1fa7a88ec3c017734c9d0a6b6b6117f74a610...master

Your commit was automatically rebased without conflicts.

Pushed as commit cd0678f.

💡 You may see a message that your pull request was closed with unmerged commits. This can be safely ignored.