New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

lazy secure random #3723

Merged
merged 2 commits into from Mar 11, 2016

Conversation

Projects
None yet
3 participants
@kares
Member

kares commented Mar 10, 2016

ThreadContext.secureRandom field was introduced as an optimization for the SecureRandomLibrary
... however as its a final field initialized on new ThreadContext all Ruby thread allocations potentially pay the cost of creating a secure random generator. which under some circumstances might get slow as they all seed themselves using a shared seeder (from a single source of entropy e.g. /dev/random under *nix).

numbers show a noticeable improvement on Ubuntu (rng-pack installed thus I rarely deplete /dev/random) :

100_000 threads BEFORE :

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    12.840000   6.640000  19.480000 ( 10.336762)
Thread.start [100000x]  12.490000   5.760000  18.250000 (  9.539517)
------------------------------------------------ total: 37.730000sec

                             user     system      total        real
Thread.new [100000x]    12.850000   5.690000  18.540000 (  9.667767)
Thread.start [100000x]  12.280000   5.830000  18.110000 (  9.503311)

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    12.190000   5.930000  18.120000 (  9.515859)
Thread.start [100000x]  12.030000   5.880000  17.910000 (  9.406770)
------------------------------------------------ total: 36.030000sec

                             user     system      total        real
Thread.new [100000x]    12.270000   5.920000  18.190000 (  9.577951)
Thread.start [100000x]  11.800000   5.690000  17.490000 (  9.167615)

100_000 threads AFTER :

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    11.040000   5.560000  16.600000 (  8.708792)
Thread.start [100000x]  10.740000   5.520000  16.260000 (  8.400990)
------------------------------------------------ total: 32.860000sec

                             user     system      total        real
Thread.new [100000x]    10.840000   5.620000  16.460000 (  8.506339)
Thread.start [100000x]  10.460000   5.560000  16.020000 (  8.323819)

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    10.990000   5.540000  16.530000 (  8.610879)
Thread.start [100000x]  10.960000   5.200000  16.160000 (  8.449772)
------------------------------------------------ total: 32.690000sec

                             user     system      total        real
Thread.new [100000x]    11.170000   5.590000  16.760000 (  8.749497)
Thread.start [100000x]  11.120000   5.370000  16.490000 (  8.661327)

with 10_000 threads :

Rehearsal ---------------------------------------------------------
Thread.new [10000x]     1.650000   0.710000   2.360000 (  1.182017)
Thread.start [10000x]   1.240000   0.560000   1.800000 (  0.940092)
------------------------------------------------ total: 4.160000sec

                            user     system      total        real
Thread.new [10000x]     1.280000   0.550000   1.830000 (  1.026601)
Thread.start [10000x]   1.200000   0.540000   1.740000 (  0.911941)

Rehearsal ---------------------------------------------------------
Thread.new [10000x]     1.730000   0.600000   2.330000 (  1.170421)
Thread.start [10000x]   1.220000   0.550000   1.770000 (  0.947210)
------------------------------------------------ total: 4.100000sec

                            user     system      total        real
Thread.new [10000x]     1.190000   0.590000   1.780000 (  0.938240)
Thread.start [10000x]   1.180000   0.590000   1.770000 (  0.978640)
Rehearsal ---------------------------------------------------------
Thread.new [10000x]     1.350000   0.700000   2.050000 (  1.046323)
Thread.start [10000x]   1.060000   0.540000   1.600000 (  0.847617)
------------------------------------------------ total: 3.650000sec

                            user     system      total        real
Thread.new [10000x]     1.070000   0.530000   1.600000 (  0.824071)
Thread.start [10000x]   0.990000   0.580000   1.570000 (  0.802974)

Rehearsal ---------------------------------------------------------
Thread.new [10000x]     1.300000   0.780000   2.080000 (  1.046365)
Thread.start [10000x]   1.060000   0.520000   1.580000 (  0.823744)
------------------------------------------------ total: 3.660000sec

                            user     system      total        real
Thread.new [10000x]     0.990000   0.580000   1.570000 (  0.806058)
Thread.start [10000x]   1.020000   0.580000   1.600000 (  0.847847)

kares added some commits Mar 10, 2016

avoid eager secure random initialization on every new Ruby thread all…
…ocation

shaves off 10-15% of Thread.new/start : 

```
Rehearsal ----------------------------------------------------------
Thread.new [100000x]    12.800000   6.850000  19.650000 ( 10.392600)
Thread.start [100000x]  12.670000   5.680000  18.350000 (  9.625846)
------------------------------------------------ total: 38.000000sec

                             user     system      total        real
Thread.new [100000x]    12.780000   5.760000  18.540000 (  9.620000)
Thread.start [100000x]  12.490000   5.810000  18.300000 (  9.524975)
```

```
Rehearsal ----------------------------------------------------------
Thread.new [100000x]    11.040000   5.560000  16.600000 (  8.708792)
Thread.start [100000x]  10.740000   5.520000  16.260000 (  8.400990)
------------------------------------------------ total: 32.860000sec

                             user     system      total        real
Thread.new [100000x]    10.840000   5.620000  16.460000 (  8.506339)
Thread.start [100000x]  10.460000   5.560000  16.020000 (  8.323819)
```

@kares kares added this to the JRuby 9.1.0.0 milestone Mar 10, 2016

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 10, 2016

Member

We may want to make the field volatile to force a memory barrier after creating and assigning the SecureRandom, but that's only necessary if SecureRandom has any construction-time race issues (i.e. non-volatile state that needs to be initialized together).

Otherwise, 👍.

Member

headius commented Mar 10, 2016

We may want to make the field volatile to force a memory barrier after creating and assigning the SecureRandom, but that's only necessary if SecureRandom has any construction-time race issues (i.e. non-volatile state that needs to be initialized together).

Otherwise, 👍.

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 10, 2016

Member

Oh, it might also be interesting to see how it looks when entropy is not being fed or the system is in an entropy-starved situation. Most people I know don't have entropy-feeding services installed, so that would be a more typical case.

Note also that Fiber and Enumerator.next instantiation both require ThreadContext instantiation. Enumerator#next could be greatly improved by reducing TC init costs.

Member

headius commented Mar 10, 2016

Oh, it might also be interesting to see how it looks when entropy is not being fed or the system is in an entropy-starved situation. Most people I know don't have entropy-feeding services installed, so that would be a more typical case.

Note also that Fiber and Enumerator.next instantiation both require ThreadContext instantiation. Enumerator#next could be greatly improved by reducing TC init costs.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 10, 2016

Member

@headius will try to starve my /dev/random although I only seen those issues in cloud installs.

does a ThreadContext really need a volatile marker - its accessed by a single thread 99% of time, when its not the non-owning thread won't try to read any fields (mostly seen thisContext == storeContext)?

Member

kares commented Mar 10, 2016

@headius will try to starve my /dev/random although I only seen those issues in cloud installs.

does a ThreadContext really need a volatile marker - its accessed by a single thread 99% of time, when its not the non-owning thread won't try to read any fields (mostly seen thisContext == storeContext)?

@headius

This comment has been minimized.

Show comment
Hide comment
@headius

headius Mar 10, 2016

Member

@kares Good point about TC being thread-local anyway; I guess volatility is not really a requirement then.

Member

headius commented Mar 10, 2016

@kares Good point about TC being thread-local anyway; I guess volatility is not really a requirement then.

@kares

This comment has been minimized.

Show comment
Hide comment
@kares

kares Mar 11, 2016

Member

tried depleting cat /dev/random (locally) but I'm not hitting serious slow-down - although there is some:

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    12.840000   5.720000  18.560000 ( 10.274086)
Thread.start [100000x]  12.360000   6.080000  18.440000 ( 10.321675)
------------------------------------------------ total: 37.000000sec

                             user     system      total        real
Thread.new [100000x]    12.850000   6.230000  19.080000 ( 11.437208)
Thread.start [100000x]  12.440000   5.960000  18.400000 ( 10.872105)

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    13.100000   5.950000  19.050000 ( 10.563208)
Thread.start [100000x]  13.010000   5.940000  18.950000 ( 10.514102)
------------------------------------------------ total: 38.000000sec

                             user     system      total        real
Thread.new [100000x]    13.280000   5.790000  19.070000 ( 10.620497)
Thread.start [100000x]  12.360000   5.740000  18.100000 ( 10.353440)
Member

kares commented Mar 11, 2016

tried depleting cat /dev/random (locally) but I'm not hitting serious slow-down - although there is some:

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    12.840000   5.720000  18.560000 ( 10.274086)
Thread.start [100000x]  12.360000   6.080000  18.440000 ( 10.321675)
------------------------------------------------ total: 37.000000sec

                             user     system      total        real
Thread.new [100000x]    12.850000   6.230000  19.080000 ( 11.437208)
Thread.start [100000x]  12.440000   5.960000  18.400000 ( 10.872105)

Rehearsal ----------------------------------------------------------
Thread.new [100000x]    13.100000   5.950000  19.050000 ( 10.563208)
Thread.start [100000x]  13.010000   5.940000  18.950000 ( 10.514102)
------------------------------------------------ total: 38.000000sec

                             user     system      total        real
Thread.new [100000x]    13.280000   5.790000  19.070000 ( 10.620497)
Thread.start [100000x]  12.360000   5.740000  18.100000 ( 10.353440)
} catch (Exception e) {
trySHA1PRNG = false;
sr = new SecureRandom();
public SecureRandom getSecureRandom() {

This comment has been minimized.

@nirvdrum

nirvdrum Mar 11, 2016

Contributor

It looks like you're relying on ThreadContext effectively being thread-local to avoid any locking. It might be good to note why this method lacks any synchronization, since it looks like a bug upon first inspection.

@nirvdrum

nirvdrum Mar 11, 2016

Contributor

It looks like you're relying on ThreadContext effectively being thread-local to avoid any locking. It might be good to note why this method lacks any synchronization, since it looks like a bug upon first inspection.

This comment has been minimized.

@kares

kares Mar 11, 2016

Member

thanks, its basically the nature of the whole class being assumed to be "single-threaded".
it has some (all non-final) non-volatile fields with non-synchronized getters/setters already.

@kares

kares Mar 11, 2016

Member

thanks, its basically the nature of the whole class being assumed to be "single-threaded".
it has some (all non-final) non-volatile fields with non-synchronized getters/setters already.

This comment has been minimized.

@headius

headius Mar 11, 2016

Member

It's also not a concern unless it's a bad thing to get extra instantiations here. My concern about volatility was just to force SecureRandom state has been published; if two get created, it's not a big deal.

@headius

headius Mar 11, 2016

Member

It's also not a concern unless it's a bad thing to get extra instantiations here. My concern about volatility was just to force SecureRandom state has been published; if two get created, it's not a big deal.

This comment has been minimized.

@nirvdrum

nirvdrum Mar 11, 2016

Contributor

Yeah, fair enough. I meant just having a comment so others don't think it's problematic. But if that's the nature of the class, that's fine too.

@nirvdrum

nirvdrum Mar 11, 2016

Contributor

Yeah, fair enough. I meant just having a comment so others don't think it's problematic. But if that's the nature of the class, that's fine too.

kares added a commit that referenced this pull request Mar 11, 2016

@kares kares merged commit 7583943 into jruby:master Mar 11, 2016

1 of 2 checks passed

continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment