code review 13256052: crypto/rand: use common buffer code on all systems

crypto/rand: use common buffer code on all systems

Buffering is needed on Windows to avoid calling CryptGenRandom for each
request. This change also simplifies unix code by moving the mutex to a
single common type.

[mxk, 2013-09-16 03:20:11]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://code.google.com/p/go

[brainman, 2013-09-16 04:00:05]

LGTM

Will have to wait for the brain trust to approve.

Alex

[r, 2013-09-16 05:10:39]

Although I am favorably inclined this does not address a documented issue.

[Dominik Honnef, 2013-09-16 05:27:00]

Semi-documented at
https://groups.google.com/d/msg/golang-dev/7JmxHbAyr-s/ZrIf1WnZ6_kJ

[r, 2013-09-16 05:30:37]

That doesn't count. We're in a release phase and if we don't follow
the rules there's no point in having them.

-rob

[brainman, 2013-09-16 05:37:38]

On 2013/09/16 05:30:37, r wrote:
> 

Fair enough. We'll have to wait till 1.2 is out. @mxk, please ping us then.

Alex

[rsc, 2013-09-16 15:18:02]

not lgtm

sorry, too many lines changing for the freeze. i am also a little skeptical
about this. is the C function we're calling on windows really that terrible?

[mxk, 2013-09-16 16:29:30]

Benchmarks are below. Buffering makes a significant difference for
small reads, but I don't know if it's the function itself or just the
syscall overhead. I only noticed this because my encryption code for
the sqlite3 package generates a random 16-byte IV every time a
database page is written to disk.

Would a smaller Windows-only version be acceptable or should I just
wait until the freeze is over?

benchmark              old ns/op    new ns/op    delta
BenchmarkReader1            1273          124  -90.26%
BenchmarkReader4            1285          137  -89.34%
BenchmarkReader16           1314          198  -84.93%
BenchmarkReader64           1477          445  -69.87%
BenchmarkReader256          2224         1418  -36.24%
BenchmarkReader1024         5228         5326   +1.87%
BenchmarkReader4096        16881        16751   -0.77%

benchmark               old MB/s     new MB/s  speedup
BenchmarkReader1            0.79         8.00   10.13x
BenchmarkReader4            3.11        29.04    9.34x
BenchmarkReader16          12.18        80.60    6.62x
BenchmarkReader64          43.33       143.81    3.32x
BenchmarkReader256        115.10       180.52    1.57x
BenchmarkReader1024       195.85       192.25    0.98x
BenchmarkReader4096       242.63       244.51    1.01x

[brainman, 2013-09-17 02:03:57]

On 2013/09/16 15:18:02, rsc wrote:
> not lgtm
> 
> sorry, too many lines changing for the freeze. ...

That is fine, we'll wait till after 1.2.

> ... i am also a little skeptical
> about this. 

I think, regardless of speed improvements, we should make this change. Why rand
generator is buffered on unix and not buffered on windows?

> ... is the C function we're calling on windows really that terrible?

It also could be really fast, and we just see the cost of syscall ...

On 2013/09/16 16:29:30, mxk wrote:
> Benchmarks are below. ...

If we are to proceed with this CL, please include your benchmarks.

> ... I don't know if it's the function itself or just the
> syscall overhead. ...

You could write a benchmark calling simple Windows API that does not do much
(for example GetLastError), and compare it with calling CryptAcquireContext.

Alex

[mxk, 2013-09-17 12:45:56]

On Mon, Sep 16, 2013 at 10:03 PM,  <alex.brainman@gmail.com> wrote:
> If we are to proceed with this CL, please include your benchmarks.

Done.

> You could write a benchmark calling simple Windows API that does not do
> much (for example GetLastError), and compare it with calling
> CryptAcquireContext.

CryptAcquireContext is only done once, I assume you meant
CryptGenRandom. Replacing it with GetLastError results in 111 ns/op
for BenchmarkReader1, while the original version is 1257 ns/op. So
it's not the syscall overhead, CryptGenRandom is just slow.

[brainman, 2013-09-18 05:38:01]

On 2013/09/17 12:45:56, mxk wrote:
> ... please include your benchmarks.
> 
> Done.

Thank you. Still LGTM.

> ... I assume you meant
> CryptGenRandom. ...

You assumed right. I misspoke.

> Replacing it with GetLastError results in 111 ns/op
> for BenchmarkReader1, while the original version is 1257 ns/op.

Cool.

> ... So
> it's not the syscall overhead, CryptGenRandom is just slow.

I am not sure why you say it is slow. It is as fast as it is. It just we
shouldn't call it 1000 times to get 1000 bytes of random data. (That is the
whole point of your CL. Isn't it?). I just tried to remove buffer in original
version of rand_unix.go, and results are very similar to what you are reporting.

I am still convinced we should make this change. If we need to wait for after
1.2, we will. So lets wait for Russ.

Alex

[gobot, 2013-12-20 05:18:40]

R=rsc@golang.org (assigned by alex.brainman@gmail.com)

[gobot, 2013-12-20 16:25:57]

Replacing golang-dev with golang-codereviews.

[bradfitz, 2014-01-16 18:44:56]

agl was recently looking into the pros and cons of buffering like this in
openssl.

It might only be worth it on Windows.

[brainman, 2014-01-17 03:13:19]

On 2014/01/16 18:44:56, bradfitz wrote:
> agl was recently looking into the pros and cons of buffering like this in
> openssl.
> 
> It might only be worth it on Windows.

I am still in favor of this change. (If anyone cares :-)

Alex

[agl1, 2014-01-17 16:25:30]

LGTM. I think this change is good.

It might also serve as a common place to build a faster solution in the face of
concurrent readers. (See https://code.google.com/p/go/issues/detail?id=7000 for
an example of where the current code is a problem.)

The worries with buffering in other languages come because of fork(). However,
Go doesn't support fork() without an immediate execve(), which will create a
fresh address space. So that's not a worry here.

[agl1, 2014-01-17 16:27:21]

p.s. will likely land next week unless there are objections.

[dfc, 2014-01-21 08:43:22]

I would prefer to see more discussion on this change.

the nil check in devReader without a mutex means that devReader is never called
concurrently, so maybe it doesn't need to defer the opening of the backing file.

Can sync.Once be used for lazy initialising the devReader backing file.

https://codereview.appspot.com/13256052/diff/19001/src/pkg/crypto/rand/rand_u...
File src/pkg/crypto/rand/rand_unix.go (right):

https://codereview.appspot.com/13256052/diff/19001/src/pkg/crypto/rand/rand_u...
src/pkg/crypto/rand/rand_unix.go:40: r.f, err = os.Open(r.name)
this is an uncommon error checking strategy. os.Open makes no guarantees about
the value of r.f if the open fails. Please use a more standard pattern.

if r.f, err = os.Open(r.name); err != nil {
    return 0, err
}

[dfc, 2014-01-21 08:43:55]

On 2014/01/21 08:43:22, dfc wrote:
> I would prefer to see more discussion on this change.
> 
> the nil check in devReader without a mutex means that devReader is never
called
> concurrently, so maybe it doesn't need to defer the opening of the backing
file.
> 
> Can sync.Once be used for lazy initialising the devReader backing file.
> 
>
https://codereview.appspot.com/13256052/diff/19001/src/pkg/crypto/rand/rand_u...
> File src/pkg/crypto/rand/rand_unix.go (right):
> 
>
https://codereview.appspot.com/13256052/diff/19001/src/pkg/crypto/rand/rand_u...
> src/pkg/crypto/rand/rand_unix.go:40: r.f, err = os.Open(r.name)
> this is an uncommon error checking strategy. os.Open makes no guarantees about
> the value of r.f if the open fails. Please use a more standard pattern.
> 
> if r.f, err = os.Open(r.name); err != nil {
>     return 0, err
> }

Also, if this CL aims to improve performance, please include before and after
benchmarks using go/misc/benchcmp.

Cheers

Dave

[rsc, 2014-04-17 02:37:08]

R=close

Please run 'hg mail' after Go 1.3 is out and it will reopen this CL for us.
Thanks.