Re: [External] : Re: RFR: 8199318: add idempotent copy operation for Map.Entry
> De: "John Rose"
> À: "Remi Forax"
> Cc: "Peter Levart" , "Rémi Forax"
> , "core-libs-dev"
> 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:fo...@univ-mlv.fr | fo...@univ-mlv.fr ]
> wrote:
>>> De: "John Rose" mailto:r.r...@oracle.com | r.r...@oracle.com ] >
>>> À: "Remi Forax" < [ mailto:fo...@univ-mlv.fr | fo...@univ-mlv.fr ] >
>>> Cc: "Peter Levart" < [ mailto:peter.lev...@gmail.com |
>>> peter.lev...@gmail.com ]
>>> >, "Rémi Forax" < [ mailto:fo...@openjdk.java.net | fo...@openjdk.java.net
>>> >] >,
>>> "core-libs-dev" < [ mailto:core-libs-dev@openjdk.java.net |
>>> core-libs-dev@openjdk.java.net ] >
>>> Envoyé: Jeudi 3 Juin 2021 22:51:28
>>> Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry
>>> ...
>>> interface ComparableRecord>
>>> extends Comparable { … }
>>> record Foo(int x, String y) implements ComparableRecord { … }
>>> [ 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 > extends Comparable< T > {
species-static Comparator 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
Re: [External] : Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Jun 3, 2021, at 3:17 PM, fo...@univ-mlv.fr<mailto:fo...@univ-mlv.fr> wrote:
De: "John Rose" mailto:r.r...@oracle.com>>
À: "Remi Forax" mailto:fo...@univ-mlv.fr>>
Cc: "Peter Levart" mailto:peter.lev...@gmail.com>>,
"Rémi Forax" mailto:fo...@openjdk.java.net>>,
"core-libs-dev"
mailto:core-libs-dev@openjdk.java.net>>
Envoyé: Jeudi 3 Juin 2021 22:51:28
Objet: Re: RFR: 8199318: add idempotent copy operation for Map.Entry
...
interface ComparableRecord>
extends Comparable { … }
record Foo(int x, String y) implements ComparableRecord { … }
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
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
> De: "John Rose"
> À: "Remi Forax" < fo...@univ-mlv.fr >
> Cc: "Peter Levart" < peter.lev...@gmail.com >, "Rémi Forax" <
> fo...@openjdk.java.net >, "core-libs-dev" < core-libs-dev@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:fo...@univ-mlv.fr |
> fo...@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>
> extends Comparable { … }
> record Foo(int x, String y) implements ComparableRecord { … }
> [ 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
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
> De: "John Rose"
> À: "Remi Forax"
> Cc: "Peter Levart" , "Rémi Forax"
> , "core-libs-dev"
> 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:fo...@univ-mlv.fr |
> fo...@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>
> extends Comparable { … }
> record Foo(int x, String y) implements ComparableRecord { … }
> [ 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> extends
Comparable {
@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> COMPARE_TO_METHODS = new
ClassValue<>() {
@Override
protected Comparator computeValue(Class type) {
return Stream.of(type.getRecordComponents())
.map(component -> {
var accessor = component.getAccessor();
return Comparator.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 { }
var list = Stream.of(new Foo(2, "foo"), new Foo(2, "bar")) .sorted().toList();
System.out.println(list);
}
}
> — John
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Jun 3, 2021, at 12:46 PM, Remi Forax
mailto:fo...@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>
extends Comparable { … }
record Foo(int x, String y) implements ComparableRecord { … }
http://cr.openjdk.java.net/~jrose/draft/ComparableRecord.java
— John
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
- Mail original - > De: "Peter Levart" > À: "Rémi Forax" , "core-libs-dev" > > 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
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? Peter
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 17:54:06 GMT, Stuart Marks wrote: >> 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. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Tiny editorial tweaks. Marked as reviewed by dfuchs (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 18:07:55 GMT, Daniel Fuchs wrote:
>> Stuart Marks has updated the pull request incrementally with one additional
>> commit since the last revision:
>>
>> Tiny editorial tweaks.
>
> src/java.base/share/classes/java/util/Map.java line 396:
>
>> 394:
>> 395: /**
>> 396: * A map entry (key-value pair). The Entry may be unmodifiable, or
>> the
>
> In that case then maybe it should be `{@code Entry}` in both places where
> `entry` was replaced with `Entry` ?
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.
-
PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 17:54:06 GMT, Stuart Marks wrote: >> 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. > > Stuart Marks has updated the pull request incrementally with one additional > commit since the last revision: > > Tiny editorial tweaks. Marked as reviewed by psandoz (Reviewer). - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
On Wed, 2 Jun 2021 17:54:06 GMT, Stuart Marks wrote:
>> 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.
>
> Stuart Marks has updated the pull request incrementally with one additional
> commit since the last revision:
>
> Tiny editorial tweaks.
src/java.base/share/classes/java/util/Map.java line 396:
> 394:
> 395: /**
> 396: * A map entry (key-value pair). The Entry may be unmodifiable, or
> the
In that case then maybe it should be `{@code Entry}` in both places where
`entry` was replaced with `Entry` ?
-
PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry [v2]
> 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. Stuart Marks has updated the pull request incrementally with one additional commit since the last revision: Tiny editorial tweaks. - Changes: - all: https://git.openjdk.java.net/jdk/pull/4295/files - new: https://git.openjdk.java.net/jdk/pull/4295/files/841a154c..c67b6445 Webrevs: - full: https://webrevs.openjdk.java.net/?repo=jdk=4295=01 - incr: https://webrevs.openjdk.java.net/?repo=jdk=4295=00-01 Stats: 3 lines in 2 files changed: 0 ins; 0 del; 3 mod Patch: https://git.openjdk.java.net/jdk/pull/4295.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4295/head:pull/4295 PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 17:12:21 GMT, Stuart Marks wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 07:35:25 GMT, Rémi Forax wrote: > 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. - PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 00:39:25 GMT, Stuart Marks wrote: > 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. I read through the javadoc and API notes and it all looks reasonable. - Marked as reviewed by alanb (Reviewer). PR: https://git.openjdk.java.net/jdk/pull/4295
Re: RFR: 8199318: add idempotent copy operation for Map.Entry
On Wed, 2 Jun 2021 00:39:25 GMT, Stuart Marks wrote: > 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. 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. - PR: https://git.openjdk.java.net/jdk/pull/4295
RFR: 8199318: add idempotent copy operation for Map.Entry
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. - Commit messages: - More spec and test tweaks. - Fix up tests. - Spec wordsmithing. - Update specs. Add basic tests. - 8199318: add idempotent copy operation for Map.Entry Changes: https://git.openjdk.java.net/jdk/pull/4295/files Webrev: https://webrevs.openjdk.java.net/?repo=jdk=4295=00 Issue: https://bugs.openjdk.java.net/browse/JDK-8199318 Stats: 141 lines in 3 files changed: 120 ins; 2 del; 19 mod Patch: https://git.openjdk.java.net/jdk/pull/4295.diff Fetch: git fetch https://git.openjdk.java.net/jdk pull/4295/head:pull/4295 PR: https://git.openjdk.java.net/jdk/pull/4295
