On Fri, Jun 14, 2024 at 4:13 PM Fabrice Benhamouda <notificati...@github.com> wrote:
> Thanks @psteitz <https://github.com/psteitz> ! I am not sure where is the > best place to post the following comment. If you think it’s more > appropriate to post it to the mailing list, let me know and I will post it > there. > > To address the point "1) gen time performance cost", I’ve written some > optimizations on top of the current PR: garydgregory#2 > <https://github.com/garydgregory/commons-lang/pull/2> > > Benchmarking using JMH with JDK 8 on an AWS c6a.xlarge instance with > cryptographic providers SUN/NativePRNG, SUN/SHA1PRNG, ACCP > <https://github.com/corretto/amazon-corretto-crypto-provider> 1.6, and > ACCP <https://github.com/corretto/amazon-corretto-crypto-provider> 2.3, > we have the following results. Compared to the current state (with the > default java.util.Random/ThreadLocalRandom number generator), when any of > the above SecureRandom is used with the optimized code, the time to > generate random alphabetic, alphanumeric, and numeric strings of at least > 16 characters is less than three times slower (and sometimes even 5x > faster). For 4-character strings, we observe a slow-down of at most 8x. > > If the optimized RandomStringUtils is called concurrently in multiple > threads, the SUN/NativePrng SecureRandom implementation (the default on > Linux) may be slower as calls are serialized (due to access to/dev/random. > This is the use case I would be most concerned with. Many web applications use this class in multi-threaded app server settings. I would not use the class in web apps after this change for that reason. If we do make the change, we should make it clear in the class javadoc that it is using SecureRandom and prone to occasional stalls. Since there was no discussion about why this change is being made, I will ask again why it is being proposed. The only rationale that I can see is that somehow some users are using the class to generate things that should be cryptographically secure, like passwords or hash salt, so to protect these users, we make all other users take a potential performance hit. Much better, IMO, would be to make the recommended improvements in performance to the vanilla Random-based impl and add a new class or somehow shoehorn in a secure impl for users that want / need that. Phil This seems an unlikely scenario in practice. In addition, switching to ACCP 2 for SecureRandom would fix the performance issue in that case. > — > Reply to this email directly, view it on GitHub > <https://github.com/apache/commons-lang/pull/1235#issuecomment-2168869983>, > or unsubscribe > <https://github.com/notifications/unsubscribe-auth/AALJJ2W2FHEF7BLURVCM3HDZHN2LFAVCNFSM6AAAAABJJE3MF6VHI2DSMVQWIX3LMV43OSLTON2WKQ3PNVWWK3TUHMZDCNRYHA3DSOJYGM> > . > You are receiving this because you were mentioned.Message ID: > <apache/commons-lang/pull/1235/c2168869...@github.com> >
