[golang-dev] Re: code review 7543043: runtime: faster & safer hash function (issue 7543043)

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/alg.c
File src/pkg/runtime/alg.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/alg.c#newcode483
src/pkg/runtime/alg.c:483: if ((runtime·cpuid_ecx & (1 << 25)) != 0 &&
// aes (aesenc)
no space after if (etc.) here and elsewhere (sorry)

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/alg.c#newcode498
src/pkg/runtime/alg.c:498: if (n > HashRandomBytes) n = HashRandomBytes;
two lines please.

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/mapspeed_test.go
File src/pkg/runtime/mapspeed_test.go (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/mapspeed_test.go#newcode24
src/pkg/runtime/mapspeed_test.go:24: for i := 0; i < b.N; i++ {
you want to reset the benchmark timer after the setup, in all
benchmarks.

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_darwin.c
File src/pkg/runtime/thread_darwin.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_darwin.c#newcode71
src/pkg/runtime/thread_darwin.c:71: static byte
urandom_data[HashRandomBytes];
this isn't really about threading (see comment above). maybe put it in a
separate function with a good name.

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_freebsd.c
File src/pkg/runtime/thread_freebsd.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_freebsd.c#newcode117
src/pkg/runtime/thread_freebsd.c:117: if (runtime·startup_random_data ==
nil) {
ditto re: function

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_linux.c
File src/pkg/runtime/thread_linux.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_linux.c#newcode162
src/pkg/runtime/thread_linux.c:162: if (runtime·startup_random_data ==
nil) {
ditto re: function.

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_netbsd.c
File src/pkg/runtime/thread_netbsd.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_netbsd.c#newcode182
src/pkg/runtime/thread_netbsd.c:182: if (runtime·startup_random_data ==
nil) {
ditto re: function

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_openbsd.c
File src/pkg/runtime/thread_openbsd.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_openbsd.c#newcode161
src/pkg/runtime/thread_openbsd.c:161: if (runtime·startup_random_data ==
nil) {
ditto re: function

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_windows.c
File src/pkg/runtime/thread_windows.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_windows.c#newcode89
src/pkg/runtime/thread_windows.c:89: if (runtime·startup_random_data ==
nil) {
ditto re: function.

if you do this uniformly, the hash initialization will all be under one
function, clearly identified by name, but different across cpu/os.

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/vdso_linux_amd64.c
File src/pkg/runtime/vdso_linux_amd64.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/vdso_linux_amd64.c#newcode329
src/pkg/runtime/vdso_linux_amd64.c:329: if (elf_auxv[i].a_type ==
AT_RANDOM) {
ditto i think but i'm not sure - this is beyond my ken.

https://codereview.appspot.com/7543043/

Despite the file name, osinit is about per-process os startup, not just
a single thread.



https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_darwin.c
File src/pkg/runtime/thread_darwin.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_darwin.c#newcode71
src/pkg/runtime/thread_darwin.c:71: static byte
urandom_data[HashRandomBytes];
Please keep it here. We can rename the file if we need to.

https://codereview.appspot.com/7543043/

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_darwin.c
File src/pkg/runtime/thread_darwin.c (right):

https://codereview.appspot.com/7543043/diff/58001/src/pkg/runtime/thread_darwin.c#newcode71
src/pkg/runtime/thread_darwin.c:71: static byte
urandom_data[HashRandomBytes];
I said separate function, not separate file. i don't see the problem
with separating it out so it's comparable across operating systems.

https://codereview.appspot.com/7543043/

Sorry, I can't see the files anymore. Rietveld keeps giving me chunk
mismatch errors.

The osinit function is the API exported by this file for operating
system-specific initialization. As long as the function is static and
called from osinit, sure.

That's my intention. The theory is that this is messy goo that each
guy does differently and it would be nice to have a separate function
for it, for grouping and clarity. It would be a static function and
called by osinit only.

sgtm

I've made a separate function and called it from hashinit, not osinit.
That way we don't need to call it if we don't have the aes instructions.

LGTM

Thanks very much. Looks great.
Apologies for the code formatting nits. They apply to all the copies.



https://codereview.appspot.com/7543043/diff/87001/src/pkg/runtime/thread_darwin.c
File src/pkg/runtime/thread_darwin.c (right):

https://codereview.appspot.com/7543043/diff/87001/src/pkg/runtime/thread_darwin.c#newcode72
src/pkg/runtime/thread_darwin.c:72: runtime·get_random_data(byte **rnd,
int32 *rnd_len) {
{ on next line. sorry.

https://codereview.appspot.com/7543043/diff/87001/src/pkg/runtime/thread_darwin.c#newcode74
src/pkg/runtime/thread_darwin.c:74: int32 fd =
runtime·open((uint8*)"/dev/urandom", 0 /* O_RDONLY */, 0);
Please initialize variables in separate statements, just to maintain the
old C style of the code.

Also if you declare open to take an int8* you can avoid the cast.

https://codereview.appspot.com/7543043/

*** Submitted as
https://code.google.com/p/go/source/detail?r=e3a5e1e9db71 ***

runtime: faster & safer hash function

Uses AES hardware instructions on 386/amd64 to implement
a fast hash function. Incorporates a random key to
thwart hash collision DOS attacks.
Depends on CL#7548043 for new assembly instructions.

Update issue 3885
Helps some by making hashing faster. Go time drops from
0.65s to 0.51s.

R=rsc, r, bradfitz, remyoudompheng, khr, dsymonds, minux.ma, elias.naur
CC=golang-dev
https://codereview.appspot.com/7543043


https://codereview.appspot.com/7543043/