[
https://issues.apache.org/jira/browse/RNG-176?page=com.atlassian.jira.plugin.system.issuetabpanels:comment-tabpanel&focusedCommentId=17534949#comment-17534949
]
Alex Herbert edited comment on RNG-176 at 5/12/22 9:39 PM:
-----------------------------------------------------------
I have created [PR 111|https://github.com/apache/commons-rng/pull/111] with
enhancements to the UniformRandomProvider interface. This adds default methods
for all uniform sampling and stream methods found in the JDK 17 RandomGenerator
interface. It adds default implementations for all existing methods except
nextLong. The interface is thus a functional interface and implementing a RNG
requires providing a source of random bits as a long:
{code:java}
UniformRandomProvider rng = new SecureRandom()::nextLong;
{code}
The addition of default implementations to existing interface methods triggered
an error in JApiCmp. I have tested the changes with:
* [JApiCmp|https://siom79.github.io/japicmp/]
* [JAPI Compliance Checker|https://lvc.github.io/japi-compliance-checker/]
* [revapi|https://revapi.org/revapi-site/main/index.html]
Here is what is flagged as binary incompatible by each using their default
settings across the current 1.5-SNAPSHOT:
||Problem||JApiCmp||JAPI||revapi||
|Add default method to interface|x [1]| | |
|Change interface method to default |x| | |
|Remove class method|x|x| |
|Add abstract method to enum| | |x|
h3. Add default method to interface
This is adding a new default method to an interface. It is one of the reasons
that default methods were introduced: to allow enhancement of the JDK
collections API without breaking binary compatibility. Here the settings for
JApiCmp are wrong. The other tools allow this change.
[1] There is a fix for this in Commons Parent to change the behaviour to allow
adding default methods.
h3. Change interface method to default
This is adding a default implementation for an existing interface method.
JApiCmp again flags this is binary incompatible. The other tools do not. JAPI
notes this in its report as a change with no effect. revapi has an entry for
this in the documentation that states the effect is equivalent (see
[java.method.nowDefault|https://revapi.org/revapi-java/0.26.1/differences.html#java.method.nowDefault]).
h3. Remove class method
This is removing the redundant implementations of UniformRandomProvider from
the commons-rng-core classes BaseProvider, IntProvider and LongProvider. Since
the methods have the same implementation in the interface these could be
removed. This is marked as binary incompatible by JApiCmp and JAPI. revapi
detects this as allowed. Since JAPI uses javap to decompile classes in the
package jar files it does not create a full class hierarchy for the core
module, i.e. it does not know about the default methods in the client
interface. I assume that japicmp may also not fully consider the dependencies.
I compiled a project against Commons RNG 1.4. Then updated the 1.5-SNAPSHOT to
remove the redundant methods. My project's entire test suit (with lots of
random number usage) ran without issue. However I did have to explicitly
include the 1.5-SNAPSHOT of commons-rng-core. My project POM only includes the
sampling and simple modules. Without explicit inclusion of the correct core and
client jars I had some run time errors due to linkage issues with missing
methods.
This issue will effect projects with large dependencies trees that include
versions via dependencies. As such it may be better to leave the current
methods in the abstract base providers in the core module. This results in some
small duplication in the codebase but will prevent any linkage errors from a
mismatched set of commons RNG modules.
h3. Add abstract method to enum
This was also flagged during testing. A new abstract method has been added in
commons-rng-simple NativeSeedType that is called by an existing method. Such a
change would be breaking if it was an abstract class since all derived classes
must have the method. However this is an enum. Thus the only derived classes
are also declared within the same codebase. It is not possible for any external
packages to extend the enum, therefore addition of an abstract method can be
ignored as all possible enum instances of the class would have to provide the
method to be able to compile the code.
This is in an {{internal}} package. As such the defaults for JAPI ignore this.
If the settings are changed to include-internal then this is flagged as a minor
issue.
It seems that both JAPI and revapi do not distinguish abstract methods in an
enum as separate from abstract methods in a class. JApiCmp does not flag this
as an issue (it would be
[java.method.abstractMethodAdded|https://revapi.org/revapi-java/0.26.1/differences.html#java.method.abstractMethodAdded]
for a class).
This highlights that there is not one tool that correctly identifies all
changes in the current 1.5-SNAPSHOT. This prevents changing from using japicmp
to revapi without adding custom configuration to ignore non-breaking changes in
an internal package. As such I added custom configuration to the japicmp maven
plugin to allow interface methods to change to have a default implementation.
I have not yet added documentation to the user guide to explain similarities
and differences between UniformRandomProvider and JDK 17 RandomGenerator. This
could be added a distinct section, e.g. titled 'Commons RNG vs java.util.random
(JDK 17)'. This allows the section to cover:
* UniformRandomProvider vs RandomGenerator
* JumpableUniformRandomProvider vs JumpableGenerator
* LongJumpableUniformRandomProvider vs LeapableGenerator
* X vs StreamableGenerator
* X vs SplittableGenerator
* X vs ArbitrarilyJumpableGenerator
Some of the interfaces have no equivalents. Addition of streaming functionality
may be possible using default methods added to the current interfaces.
See also [Javadoc
java.util.random|https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/package-summary.html]
was (Author: alexherbert):
I have created [PR 111|https://github.com/apache/commons-rng/pull/111] with
enhancements to the UniformRandomProvider interface. This adds default methods
for all uniform sampling and stream methods found in the JDK 17 RandomGenerator
interface. It adds default implementations for all existing methods except
nextLong. The interface is thus a functional interface and implementing a RNG
requires providing a source of random bits as a long:
{code:java}
UniformRandomProvider rng = new SecureRandom()::nextLong;
{code}
The addition of default implementations to existing interface methods triggered
an error in JApiCmp. I have tested the changes with:
* [JApiCmp|https://siom79.github.io/japicmp/]
* [JAPI Compliance Checker|https://lvc.github.io/japi-compliance-checker/]
* [revapi|https://revapi.org/revapi-site/main/index.html]
Here is what is flagged as binary incompatible by each using their default
settings across the current 1.5-SNAPSHOT:
||Problem||JApiCmp||JAPI||revapi||
|Add default method to interface|x *| | |
|Change interface method to default |x| | |
|Remove class method|x|x| |
|Add abstract method to enum| | |x|
h3. Add default method to interface
This is adding a new default method to an interface. It is one of the reasons
that default methods were introduced: to allow enhancement of the JDK
collections API without breaking binary compatibility. Here the settings for
JApiCmp are wrong. The other tools allow this change.
* There is a fix for this in Commons Parent to change the behaviour to allow
adding default methods.
h3. Change interface method to default
This is adding a default implementation for an existing interface method.
JApiCmp again flags this is binary incompatible. The other tools do not. JAPI
notes this in its report as a change with no effect. revapi has an entry for
this in the documentation that states the effect is equivalent (see
[java.method.nowDefault|https://revapi.org/revapi-java/0.26.1/differences.html#java.method.nowDefault]).
h3. Remove class method
This is removing the redundant implementations of UniformRandomProvider from
the commons-rng-core classes BaseProvider, IntProvider and LongProvider. Since
the methods have the same implementation in the interface these could be
removed. This is marked as binary incompatible by JApiCmp and JAPI. revapi
detects this as allowed. Since JAPI uses javap to decompile classes in the
package jar files it does not create a full class hierarchy for the core
module, i.e. it does not know about the default methods in the client
interface. I assume that japicmp may also not fully consider the dependencies.
I compiled a project against Commons RNG 1.4. Then updated the 1.5-SNAPSHOT to
remove the redundant methods. My projects entire test suit (with lots of random
number usage) ran without issue. However I did have to explicitly include the
1.5-SNAPSHOT of commons-rng-core. My project POM only includes the sampling and
simple modules. Without explicit inclusion of the correct core and client jars
I had some run time errors due to linkage issues with missing methods.
This issue will effect projects with large dependencies trees that include
versions via dependencies. As such it may be better to leave the current
methods in the abstract base providers in the core module. This results in some
small duplication in the codebase but will prevent any linkage errors from a
mismatched set of commons RNG modules.
h3. Add abstract method to enum
This was also flagged during testing. A new abstract method has been added in
commons-rng-simple NativeSeedType that is called by an existing method. Such a
change would be breaking if it was an abstract class since all derived classes
must have the method. However this is an enum. Thus the only derived classes
are also declared within the same codebase. It is not possible for any external
packages to extend the enum, therefore addition of an abstract method can be
ignored as all possible enum instances of the class would have to provide the
method to be able to compile the code.
This is in an {{internal}} package. As such the defaults for JAPI ignore this.
If the settings are changed to include-internal then this is flagged as a minor
issue.
It seems that both JAPI and revapi do not distinguish abstract methods in an
enum as separate from abstract methods in a class. JApiCmp does not flag this
as an issue (it would be
[java.method.abstractMethodAdded|https://revapi.org/revapi-java/0.26.1/differences.html#java.method.abstractMethodAdded]
for a class).
This highlights that there is not one tool that correctly identifies all
changes in the current 1.5-SNAPSHOT. This prevents changing from using japicmp
to revapi without adding custom configuration to ignore non-breaking changes in
an internal package. As such I added custom configuration to the japicmp maven
plugin to allow interface methods to change to have a default implementation.
I have not yet added documentation to the user guide to explain similarities
and differences between UniformRandomProvider and JDK 17 RandomGenerator. This
could be added a distinct section, e.g. titled 'Commons RNG vs java.util.random
(JDK 17)'. This allows the section to cover:
* UniformRandomProvider vs RandomGenerator
* JumpableUniformRandomProvider vs JumpableGenerator
* LongJumpableUniformRandomProvider vs LeapableGenerator
* X vs StreamableGenerator
* X vs SplittableGenerator
* X vs ArbitrarilyJumpableGenerator
Some of the interfaces have no equivalents. Addition of streaming functionality
may be possible using default methods added to the current interfaces.
See also [Javadoc
java.util.random|https://docs.oracle.com/en/java/javase/17/docs/api/java.base/java/util/random/package-summary.html]
> Enhance the UniformRandomProvider interface with extra methods and default
> implementations
> ------------------------------------------------------------------------------------------
>
> Key: RNG-176
> URL: https://issues.apache.org/jira/browse/RNG-176
> Project: Commons RNG
> Issue Type: New Feature
> Affects Versions: 1.4
> Reporter: Alex Herbert
> Assignee: Alex Herbert
> Priority: Major
> Time Spent: 20m
> Remaining Estimate: 0h
>
> JDK 17 introduced the {{RandomGenerator}} interface with the following
> methods:
> {code:java}
> DoubleStream doubles();
> DoubleStream doubles(double randomNumberOrigin, double randomNumberBound);
> DoubleStream doubles(long streamSize);
> DoubleStream doubles(long streamSize, double randomNumberOrigin,
> double randomNumberBound);
> IntStream ints();
> IntStream ints(int randomNumberOrigin, int randomNumberBound);
> IntStream ints(long streamSize);
> IntStream ints(long streamSize, int randomNumberOrigin,
> int randomNumberBound);
> LongStream longs();
> LongStream longs(long randomNumberOrigin, long randomNumberBound);
> LongStream longs(long streamSize);
> LongStream longs(long streamSize, long randomNumberOrigin,
> long randomNumberBound);
> boolean nextBoolean();
> void nextBytes(byte[] bytes);
> float nextFloat();
> float nextFloat(float bound);
> float nextFloat(float origin, float bound);
> double nextDouble();
> double nextDouble(double bound);
> double nextDouble(double origin, double bound);
> int nextInt();
> int nextInt(int bound);
> int nextInt(int origin, int bound);
> long nextLong();
> long nextLong(long bound);
> long nextLong(long origin, long bound);
> double nextGaussian();
> double nextGaussian(double mean, double stddev);
> double nextExponential();
> {code}
> The only method that is *non-default* is {{{}nextLong{}}}. This allows a new
> generator to be simply implemented by providing the source of randomness as
> 64-bit longs.
> The {{UniformRandomProvider}} interface can be expanded to include these
> generation methods. Using Java 8 default interface methods will not require
> any changes to generators currently implementing the interface.
> I propose to:
> # Add the new methods for streams and numbers in a range.
> # Add default implementations of the current API. These can be extracted
> from the o.a.c.rng.core.BaseProvider implementations.
> # Remove the implementations in o.a.c.rng.core.BaseProvider. This change
> would be binary compatible.
> The base classes in commons core for 32-bit and 64-bit sources of randomness,
> IntProvider and LongProvider, can be updated suitably to only override the
> default interface methods where they can be more efficiently implemented
> given the source of randomness. This applies to:
> ||Source||Update||Details||
> |int|nextBytes|Use nextInt() for the source of bytes|
> | |nextBoolean|Use a cached int for the randomness|
> | |nextInt|Directly supply the int rather than using 32-bits from nextLong()|
> | |nextDouble|Optimise the bits used from two ints for the 53-bits required
> for the double.|
> |long|nextInt; nextBoolean|Use a cached long for the randomness|
> h3. Note 1
> The UniformRandomProvider also has the method:
> {code:java}
> void nextBytes(byte[] bytes,
> int start,
> int len);
> {code}
> This can also have a default implementation using the output from nextLong().
> h3. Note 2
> The methods to generate an exponential and Gaussian are already implemented
> in the {{commons-rng-sampling}} module.
> java.util.Random has a nextGaussian() method and so this method appears to be
> for backward compatibility with legacy Java code. The method is implemented
> using a modified Ziggurat sampler which uses an exponential sampler for the
> long tail. The API has thus exposed the exponential sampling method that is
> used internally in the nextGaussian implementation.
> With no backward compatibility requirements the Commons RNG interface can
> avoid the distribution sampling methods. Users should select an appropriate
> sampler from the sampling module.
>
--
This message was sent by Atlassian Jira
(v8.20.7#820007)
