code review 165950043: runtime: allocate a list of small integers that interfa...

runtime: allocate a list of small integers that interfaces can point to.

This change avoids allocations in the case where small integers
are stored in an interface.

benchmark                          old ns/op      new ns/op      delta
BenchmarkBinaryTree17              2480757900     2448184501     -1.31%
BenchmarkFannkuch11                2404707156     2369879305     -1.45%
BenchmarkFmtFprintfEmpty           63.1           62.8           -0.48%
BenchmarkFmtFprintfString          173            174            +0.58%
BenchmarkFmtFprintfInt             180            156            -13.33%
BenchmarkFmtFprintfIntInt          288            255            -11.46%
BenchmarkFmtFprintfPrefixedInt     253            233            -7.91%
BenchmarkFmtFprintfFloat           350            351            +0.29%
BenchmarkFmtManyArgs               1088           985            -9.47%
BenchmarkGobDecode                 8972467        8907096        -0.73%
BenchmarkGobEncode                 7710615        7711389        +0.01%
BenchmarkGzip                      353718004      351409270      -0.65%
BenchmarkGunzip                    87743203       87268032       -0.54%
BenchmarkHTTPClientServer          67024          69889          +4.27%
BenchmarkJSONEncode                17207806       17357425       +0.87%
BenchmarkJSONDecode                65649590       66194083       +0.83%
BenchmarkMandelbrot200             3731891        3734023        +0.06%
BenchmarkGoParse                   3504030        3508157        +0.12%
BenchmarkRegexpMatchEasy0_32       113            115            +1.77%
BenchmarkRegexpMatchEasy0_1K       284            296            +4.23%
BenchmarkRegexpMatchEasy1_32       99.5           98.3           -1.21%
BenchmarkRegexpMatchEasy1_1K       810            818            +0.99%
BenchmarkRegexpMatchMedium_32      186            184            -1.08%
BenchmarkRegexpMatchMedium_1K      63485          62011          -2.32%
BenchmarkRegexpMatchHard_32        3097           3124           +0.87%
BenchmarkRegexpMatchHard_1K        100096         101571         +1.47%
BenchmarkRevcomp                   508628203      515428743      +1.34%
BenchmarkTemplate                  81041347       81873344       +1.03%
BenchmarkTimeParse                 383            380            -0.78%
BenchmarkTimeFormat                344            351            +2.03%

[gri, 2014-10-31 00:42:20]

https://codereview.appspot.com/165950043/diff/20001/src/runtime/iface.go
File src/runtime/iface.go (right):

https://codereview.appspot.com/165950043/diff/20001/src/runtime/iface.go#newc...
src/runtime/iface.go:150: if *(*uintptr)(elem) < uintptr(len(staticints)) &&
t.size == ptrSize {
Is t.size in bytes?

If so, should this be t.size <= ptrSize ?

A comment about the ordering of the && operands would also be useful down the
road.

[r, 2014-10-31 01:01:36]

please include before and after benchmarking results.
use 

cd test/bench/go1
go test -run XXX -bench .

https://codereview.appspot.com/165950043/diff/20001/src/fmt/fmt_test.go
File src/fmt/fmt_test.go (right):

https://codereview.appspot.com/165950043/diff/20001/src/fmt/fmt_test.go#newco...
src/fmt/fmt_test.go:830: func BenchmarkSprintfInt2(b *testing.B) {
delete this. it's a debugging dreg.

[khr, 2014-10-31 04:20:51]

Hello gri@golang.org, r@golang.org, rsc@golang.org (cc:
golang-codereviews@googlegroups.com),

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

[khr1, 2014-10-31 05:03:07]

On Thu, Oct 30, 2014 at 5:42 PM, <gri@golang.org> wrote:

>
> https://codereview.appspot.com/165950043/diff/20001/src/runtime/iface.go
> File src/runtime/iface.go (right):
>
> https://codereview.appspot.com/165950043/diff/20001/src/
> runtime/iface.go#newcode150
> src/runtime/iface.go:150: if *(*uintptr)(elem) <
> uintptr(len(staticints)) && t.size == ptrSize {
> Is t.size in bytes?
>
> If so, should this be t.size <= ptrSize ?
>
>
I don't think we can do size < ptrSize, at least not easily.  The incoming
pointer may point anywhere - if it points at a [7]byte at the end of a page
and the next page is unmapped, we're in trouble.  Similarly, we need to
check alignment before loading, on arm at least.


> A comment about the ordering of the && operands would also be useful
> down the road.
>
>
So we need to check size and alignment first.  We can't load until we've
done those checks.


> https://codereview.appspot.com/165950043/
>

[gri, 2014-10-31 05:10:22]

Good point.

It'd be interesting to see if special casing one more case (say 32bit
values) does make any more difference; I'm thinking of runes, for instance.
On Thu Oct 30 2014 at 10:03:09 PM Keith Randall <khr@google.com> wrote:

> On Thu, Oct 30, 2014 at 5:42 PM, <gri@golang.org> wrote:
>
>>
>> https://codereview.appspot.com/165950043/diff/20001/src/runtime/iface.go
>> File src/runtime/iface.go (right):
>>
>> https://codereview.appspot.com/165950043/diff/20001/src/
>> runtime/iface.go#newcode150
>> src/runtime/iface.go:150: if *(*uintptr)(elem) <
>> uintptr(len(staticints)) && t.size == ptrSize {
>> Is t.size in bytes?
>>
>> If so, should this be t.size <= ptrSize ?
>>
>>
> I don't think we can do size < ptrSize, at least not easily.  The incoming
> pointer may point anywhere - if it points at a [7]byte at the end of a page
> and the next page is unmapped, we're in trouble.  Similarly, we need to
> check alignment before loading, on arm at least.
>
>
>> A comment about the ordering of the && operands would also be useful
>> down the road.
>>
>>
> So we need to check size and alignment first.  We can't load until we've
> done those checks.
>
>
>> https://codereview.appspot.com/165950043/
>>
>
>

[gri, 2014-10-31 05:15:35]

lgtm

https://codereview.appspot.com/165950043/diff/100001/src/runtime/iface.go
File src/runtime/iface.go (right):

https://codereview.appspot.com/165950043/diff/100001/src/runtime/iface.go#new...
src/runtime/iface.go:156: }
If we don't have the value in the array we still know that it's ptrSize in size.
We could avoid the memmove in this case and replace with a direct assignment.
Not sure how expensive memmove is overall in this case.

Should be fairly easy to test.

[r, 2014-10-31 05:52:30]

https://codereview.appspot.com/165950043/diff/100001/src/runtime/iface.go
File src/runtime/iface.go (right):

https://codereview.appspot.com/165950043/diff/100001/src/runtime/iface.go#new...
src/runtime/iface.go:137: // NOTE: can't do this as a regular init() -
convT2{I,E} get called before a regular init() does.
you could use regular init if you set a boolean at the end and then checked it
in convT2E in the new if clause

[rsc, 2014-10-31 11:11:53]

not lgtm yet

Is there a reason to do this right now? It seems a bit too late in the
cycle for this unless there is a compelling need.

Instead of the init, I'd write a tiny program to write the array to a
separate file. You can use go generate. See CL 168770043 for an example
(mgc0.c for the //go:generate line, wbfat_gen.go, wbfat.go).

Russ

[khr1, 2014-10-31 15:13:07]

On Fri, Oct 31, 2014 at 4:11 AM, Russ Cox <rsc@golang.org> wrote:

> not lgtm yet
>
> Is there a reason to do this right now? It seems a bit too late in the
> cycle for this unless there is a compelling need.
>
>
This was in response to some performance regressions people were reporting
on golang-nuts.  At least it will be a good experiment - we can make the
decision after we see if it helps anyone.


> Instead of the init, I'd write a tiny program to write the array to a
> separate file. You can use go generate. See CL 168770043 for an example
> (mgc0.c for the //go:generate line, wbfat_gen.go, wbfat.go).
>
>
We could.  It is a question of whether we'd like an additional 8K in the
binary or initialize 8K at startup.  I could go either way.


> Russ
>

[rsc, 2014-10-31 15:50:57]

On Fri, Oct 31, 2014 at 11:13 AM, Keith Randall <khr@google.com> wrote:

> On Fri, Oct 31, 2014 at 4:11 AM, Russ Cox <rsc@golang.org> wrote:
>
>> not lgtm yet
>>
>> Is there a reason to do this right now? It seems a bit too late in the
>> cycle for this unless there is a compelling need.
>>
>>
> This was in response to some performance regressions people were reporting
> on golang-nuts.  At least it will be a good experiment - we can make the
> decision after we see if it helps anyone.
>

It's fine if you want to suggest people patch in the CL and try it, but I'd
rather not check it in now, absent someone who is really hurting and needs
it. We should not be changing things this fundamental this late in the
cycle without a documented, compelling need. The chance of breaking
something unintentionally and not finding it is too high. I don't see
anything wrong with this CL, of course, but that's true of the buggy ones
too. We need to draw a line.

Russ

[gri, 2014-10-31 16:54:48]

I suggested this optimization. It's a classical VM trick to ameliorate the
cost of allocation/boxing. Small integers are common, and so are containers
and parameter lists which take empty interfaces. It's late in the game, but
it seems silly to leave an easy gain like this on the table.
On Fri Oct 31 2014 at 8:51:01 AM Russ Cox <rsc@golang.org> wrote:

> On Fri, Oct 31, 2014 at 11:13 AM, Keith Randall <khr@google.com> wrote:
>
>> On Fri, Oct 31, 2014 at 4:11 AM, Russ Cox <rsc@golang.org> wrote:
>>
>>> not lgtm yet
>>>
>>> Is there a reason to do this right now? It seems a bit too late in the
>>> cycle for this unless there is a compelling need.
>>>
>>>
>> This was in response to some performance regressions people were
>> reporting on golang-nuts.  At least it will be a good experiment - we can
>> make the decision after we see if it helps anyone.
>>
>
> It's fine if you want to suggest people patch in the CL and try it, but
> I'd rather not check it in now, absent someone who is really hurting and
> needs it. We should not be changing things this fundamental this late in
> the cycle without a documented, compelling need. The chance of breaking
> something unintentionally and not finding it is too high. I don't see
> anything wrong with this CL, of course, but that's true of the buggy ones
> too. We need to draw a line.
>
> Russ
>

[rsc, 2014-10-31 17:28:30]

Changes introduce bugs. The best way we know to avoid them this is to stop
making changes this late in the cycle, however trivial they seem, unless
the reward greatly outweighs the risk.

Dmitriy proposed a variant of this change too (just store the number
directly in the pointer slot), but in both cases the change is making Go
runtime behavior depend on the actual value being stored, not just its
type, which is not something we've done before. We don't have many (any?)
value-dependent performance cliffs like that, we don't know what the
implications are, and people have already started beta testing with the
current implementation.

On the general performance topic, we leave things on the table every cycle,
because we don't get to do everything we want. But in this cycle
specifically, it is great to be leaving things on Go 1.5's table. We may
need them then.

Russ

On Fri Oct 31 2014 at 12:54:46 PM Robert Griesemer <gri@golang.org> wrote:

> I suggested this optimization. It's a classical VM trick to ameliorate the
> cost of allocation/boxing. Small integers are common, and so are containers
> and parameter lists which take empty interfaces. It's late in the game, but
> it seems silly to leave an easy gain like this on the table.
> On Fri Oct 31 2014 at 8:51:01 AM Russ Cox <rsc@golang.org> wrote:
>
>> On Fri, Oct 31, 2014 at 11:13 AM, Keith Randall <khr@google.com> wrote:
>>
>>> On Fri, Oct 31, 2014 at 4:11 AM, Russ Cox <rsc@golang.org> wrote:
>>>
>>>> not lgtm yet
>>>>
>>>> Is there a reason to do this right now? It seems a bit too late in the
>>>> cycle for this unless there is a compelling need.
>>>>
>>>>
>>> This was in response to some performance regressions people were
>>> reporting on golang-nuts.  At least it will be a good experiment - we can
>>> make the decision after we see if it helps anyone.
>>>
>>
>> It's fine if you want to suggest people patch in the CL and try it, but
>> I'd rather not check it in now, absent someone who is really hurting and
>> needs it. We should not be changing things this fundamental this late in
>> the cycle without a documented, compelling need. The chance of breaking
>> something unintentionally and not finding it is too high. I don't see
>> anything wrong with this CL, of course, but that's true of the buggy ones
>> too. We need to draw a line.
>>
>> Russ
>>
>

[gobot, 2014-12-19 05:15:06]

R=close

To the author of this CL:

The Go project has moved to Gerrit Code Review.

If this CL should be continued, please see the latest version of
https://golang.org/doc/contribute.html for instructions on
how to set up Git and the Go project's Gerrit codereview plugin,
and then create a new change with your current code.

If there has been discussion on this CL, please give a link to it
(golang.org/cl/165950043 is best) in the description in your
new CL.

Thanks very much.