code review 6137051: runtime: fix hashmap code for large key size

runtime: fix hashmap code for large key size

Fixes issue 3573.

[iant, 2012-04-30 22:06:34]

Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[dave_cheney.net, 2012-04-30 22:16:10]

Hi Ian,

Thank you for fixing this issue. Just an observation on the performance cost of
this change

core i5, darwin/amd64

odessa(~/go/src/pkg/runtime) % ~/go/misc/benchcmp {old,new}.txt
benchmark                old ns/op    new ns/op    delta
BenchmarkMapUint8Get            46           49   +5.98%
BenchmarkMapUint16Get           65           62   -5.49%
BenchmarkMapUint32Get           66           67   +1.05%
BenchmarkMapUint64Get           78           80   +2.80%

From http://codereview.appspot.com/6129057/

[rsc, 2012-05-01 02:25:11]

This is so weird.  When I added support for arbitrary equality, I
implemented support for indirect keys, along with logic to decide when
to indirect the key+value, when to indirect just the key, and when to
indirect just the value, and tests.  And it is clearly not here, but I
can't figure out where it went.

[bradfitz, 2012-05-01 02:30:02]

Did you ask the other Russes?

On Tue, May 1, 2012 at 12:24 PM, Russ Cox <rsc@golang.org> wrote:

> This is so weird.  When I added support for arbitrary equality, I
> implemented support for indirect keys, along with logic to decide when
> to indirect the key+value, when to indirect just the key, and when to
> indirect just the value, and tests.  And it is clearly not here, but I
> can't figure out where it went.
>

[rsc, 2012-05-01 02:34:01]

On Mon, Apr 30, 2012 at 22:24, Russ Cox <rsc@golang.org> wrote:
> This is so weird.  When I added support for arbitrary equality, I
> implemented support for indirect keys, along with logic to decide when
> to indirect the key+value, when to indirect just the key, and when to
> indirect just the value, and tests.  And it is clearly not here, but I
> can't figure out where it went.

Looks like one of the other Russes reverted hashmap.c
(and, conveniently, test/bigmap.go) before submitting the CL.
The changes are in patch 9 but not in patch 10 (submitted).

http://codereview.appspot.com/5451105/diff/12001/src/pkg/runtime/hashmap.c

I'd like to try putting them in instead of this CL, both because the
tests are quite a bit more extensive and because Mike Burrows
already signed off on that delta (his mail is how I found the link),
but I wonder whether they were omitted for a good reason.
I wonder if I realized a problem and meant to come back.
I'll take a closer look tomorrow.

Russ

[rsc, 2012-05-25 03:02:49]

I submitted my earlier copy of these files, finally.

[iant, 2012-05-29 19:29:01]

*** Abandoned ***