Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
> Java 17 added RandomGenerator interface. However, existing method > Collections.shuffle accepts old java.util.Random class. While since Java 19, > it's possible to use Random.from(RandomGenerator) wrapper, it would be more > convenient to provide direct overload shuffle(List list, RandomGenerator > rnd). Tagir F. Valeev has updated the pull request incrementally with one additional commit since the last revision: Remove Random -> ThreadLocalRandom change - Changes: - all: https://git.openjdk.org/jdk/pull/10520/files - new: https://git.openjdk.org/jdk/pull/10520/files/40a69fec..6fa7d942 Webrevs: - full: https://webrevs.openjdk.org/?repo=jdk&pr=10520&range=01 - incr: https://webrevs.openjdk.org/?repo=jdk&pr=10520&range=00-01 Stats: 7 lines in 1 file changed: 5 ins; 1 del; 1 mod Patch: https://git.openjdk.org/jdk/pull/10520.diff Fetch: git fetch https://git.openjdk.org/jdk pull/10520/head:pull/10520 PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Random -> ThreadLocalRandom change test/jdk/java/util/Collections/Shuffle.java line 92: > 90: throw new RuntimeException(list.getClass() + ": " + list + " > != " + copy); > 91: } > 92: } Is there a way to factor out the `shuffle` calls and thereby collapse these two methods into one? Is it worth it? I'm thinking you could pass in a `Consumer>`. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote: >> Java 17 added RandomGenerator interface. However, existing method >> Collections.shuffle accepts old java.util.Random class. While since Java 19, >> it's possible to use Random.from(RandomGenerator) wrapper, it would be more >> convenient to provide direct overload shuffle(List list, RandomGenerator >> rnd). > > Tagir F. Valeev has updated the pull request incrementally with one > additional commit since the last revision: > > Remove Random -> ThreadLocalRandom change test/jdk/java/util/Collections/Shuffle.java line 66: > 64: RandomGeneratorFactory factory = > RandomGeneratorFactory.getDefault(); > 65: list.sort(null); > 66: Collections.shuffle(list, factory.create(1)); This assumes that the default factory will accept a seed value that initializes its state so that the pseudorandom sequence is repeatable. Not an unreasonable assumption, but `create(long)` essentially says that it can ignore the seed, and `getDefault` says the algorithm may change over time. On the other hand if something does change such that the pseudorandom sequence isn't repeatable, this test will most likely fail immediately. I was poking around for an explicit way to request some kind of PRNG that's guaranteed to be started in repeatable state, but I couldn't find one. Hmmm. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Sat, 8 Oct 2022 15:35:14 GMT, Tagir F. Valeev wrote:
>> Java 17 added RandomGenerator interface. However, existing method
>> Collections.shuffle accepts old java.util.Random class. While since Java 19,
>> it's possible to use Random.from(RandomGenerator) wrapper, it would be more
>> convenient to provide direct overload shuffle(List list, RandomGenerator
>> rnd).
>
> Tagir F. Valeev has updated the pull request incrementally with one
> additional commit since the last revision:
>
> Remove Random -> ThreadLocalRandom change
src/java.base/share/classes/java/util/Collections.java line 485:
> 483: * list-iterator does not support the {@code set} operation.
> 484: * @since 20
> 485: */
It looks like this comment was copied from `shuffle(List, Random)` and
`shuffle(List)` which is fine. But since this method is now preferred over the
others, maybe we can reduce the duplication and have those other methods simply
be defined in terms of this one. We'd have to come up with the right "weasel
words" to describe the source of randomness used by `shuffle(List)`. In
addition, if you don't want to deprecate the other methods, perhaps some
wording can be found that clearly indicates this new method is now the
preferred one.
-
PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Tue, 11 Oct 2022 01:48:41 GMT, Stuart Marks wrote: >> Tagir F. Valeev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove Random -> ThreadLocalRandom change > > test/jdk/java/util/Collections/Shuffle.java line 92: > >> 90: throw new RuntimeException(list.getClass() + ": " + list + " >> != " + copy); >> 91: } >> 92: } > > Is there a way to factor out the `shuffle` calls and thereby collapse these > two methods into one? Is it worth it? I'm thinking you could pass in a > `Consumer>`. Good idea, thanks! Will do. - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Tue, 11 Oct 2022 01:56:38 GMT, Stuart Marks wrote: >> Tagir F. Valeev has updated the pull request incrementally with one >> additional commit since the last revision: >> >> Remove Random -> ThreadLocalRandom change > > test/jdk/java/util/Collections/Shuffle.java line 66: > >> 64: RandomGeneratorFactory factory = >> RandomGeneratorFactory.getDefault(); >> 65: list.sort(null); >> 66: Collections.shuffle(list, factory.create(1)); > > This assumes that the default factory will accept a seed value that > initializes its state so that the pseudorandom sequence is repeatable. Not an > unreasonable assumption, but `create(long)` essentially says that it can > ignore the seed, and `getDefault` says the algorithm may change over time. On > the other hand if something does change such that the pseudorandom sequence > isn't repeatable, this test will most likely fail immediately. I was poking > around for an explicit way to request some kind of PRNG that's guaranteed to > be started in repeatable state, but I couldn't find one. Hmmm. Oh, that's an unfortunate detail, I didn't know about this. We can explicitly request a JumpableGenerator, like "Xoshiro256PlusPlus" and then copy it via `copy()` method. What do you think? - PR: https://git.openjdk.org/jdk/pull/10520
Re: RFR: 8294693: Add Collections.shuffle overload that accepts RandomGenerator interface [v2]
On Tue, 11 Oct 2022 02:00:40 GMT, Stuart Marks wrote:
>> Tagir F. Valeev has updated the pull request incrementally with one
>> additional commit since the last revision:
>>
>> Remove Random -> ThreadLocalRandom change
>
> src/java.base/share/classes/java/util/Collections.java line 485:
>
>> 483: * list-iterator does not support the {@code set} operation.
>> 484: * @since 20
>> 485: */
>
> It looks like this comment was copied from `shuffle(List, Random)` and
> `shuffle(List)` which is fine. But since this method is now preferred over
> the others, maybe we can reduce the duplication and have those other methods
> simply be defined in terms of this one. We'd have to come up with the right
> "weasel words" to describe the source of randomness used by `shuffle(List)`.
> In addition, if you don't want to deprecate the other methods, perhaps some
> wording can be found that clearly indicates this new method is now the
> preferred one.
I updated the documentation, please take a look. Deprecation sounds not a good
idea, as if you want to use a randomness source that happens to extend Random
class (e.g., ThreadLocalRandom.current()), compiler will link it to the old
method. This is totally fine, and you should not get a deprecation warning.
-
PR: https://git.openjdk.org/jdk/pull/10520
