Hi.
Le lun. 3 juin 2019 à 16:49, Alex Herbert <alex.d.herb...@gmail.com> a écrit :
>
> Can I get a review of a PR that changes the ProviderBuilder [1]?
>
> The aim is to move the creation of the seed and the RNG into the
> RandomSourceInternal.
>
> This allows for customisation of the seed creation on a per-RNG basis.
> The reason is that some new generators in the library for v1.3 require
> the seed is non-zero. For example this is a realistic possibility when
> the seed is an array of ints of size 2.
>
> The code change has required updating the routines using the
> SeedConverter interface and adding a Seed2ArrayConverter interface:
>
> public interface Seed2ArrayConverter<IN, OUT> extends SeedConverter<IN, OUT> {
> /**
> * Converts seed from input type to output type. The output type is
> expected to be an array.
> *
> * @param seed Original seed value.
> * @param outputSize Output size.
> * @return the converted seed value.
> */
> OUT convert(IN seed, int outputSize);
> }
>
> I've moved conversion to a new enum class for seed creation and
> conversion. Previously the ProviderBuilder used maps to store all the
> conversions. These are now explicitly written as conversion methods in
> the enum. The amount of code is the same and the conversions are the
> same. However use of the enum for conversion has removed the need to
> support the Seed2ArrayConverter interface in all the converters. This
> has deprecated some methods and classes. I've not yet marked them as so
> in the PR.
>
> i.e.
>
> private static final Map<Class<?>, SeedConverter<long[], ?>> CONV_LONG_ARRAY =
> new ConcurrentHashMap<Class<?>, SeedConverter<long[], ?>>();
>
> does not have to become:
>
> private static final Map<Class<?>, Seed2ArrayConverter<long[], ?>>
> CONV_LONG_ARRAY =
> new ConcurrentHashMap<Class<?>, Seed2ArrayConverter<long[], ?>>();
>
> with the corresponding conversion as:
>
> nativeSeed = CONV_LONG_ARRAY.get(source.getSeed()).convert((long[]) seed,
> source.getSeedSize());
>
>
> Currently conversions of arrays to arrays ignore the seed size. Creation
> of a new seed is limited to a maximum of 128.
+0
See below.
> This matches the previous
> functionality. It could be changed to create the full length seed.
+0
See below.
> The
> impact would be more work done within synchronized blocks in the
> SeedFactory. I would expect the generation to be slower but the seed
> quality will be higher.
As per the previous discussion, special needs should be covered
by the user.
However, it could be construed that 128 is large enough for a casual
user, and a larger seed could also pass as a special need that the
user can provide explicitly...
So either way is fine I guess.
>
> Only creation of arrays from int/long seeds will use the seed size. This
> does not operate within a synchronized block.
>
>
> Note that RNG constructor is obtained using reflection. Previously was
> done on each invocation. However now that the method is within the
> RandomSourceInternal enum caching the constructor is a natural
> modification since it is always the same.
+1
But please rename the local variable in method "getConstructor()".[1]
;-)
> I have not done this in the
> constructor for the RandomSourceInternal enum as:
>
> - it adds overhead when building all the enums (e.g.
> RandomSourceInternal.values())
>
> - it may throw lots of different types of exception. Rather than
> catching them all in the constructor for the enum they can now be thrown
> during creation of the RNG instance so the user gets the appropriate
> stack trace.
>
>
> The change to create an array using the correct size has performance
> implications (see [2]):
>
> - For small array sizes the creation is faster
>
> - For large array sizes built using an int/long the creation is
> marginally slower (but the seed should be better as it uses the SplitMix
> algorithm rather than the self-seeding strategy of the BaseProvider [3])
Where is it done?
>
> Caching the constructor for use with reflection has improved performance.
+1
Thanks,
Gilles
>
>
> [1] https://github.com/apache/commons-rng/pull/46
>
> [2] https://issues.apache.org/jira/browse/RNG-75
>
> [3] This could be tested by creating a new generator that implements the
> self-seeding strategy as its output and running it through the stress
> test applications.
>
Gilles
[1] https://www.linguee.com/french-english/translation/con.html
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscr...@commons.apache.org
For additional commands, e-mail: dev-h...@commons.apache.org
