code review 7397056: cmd/gc: apply escape analysis results to closures.

cmd/gc: apply escape analysis results to closures.

This avoids an allocation when closures are used
as "macros", in Walk idioms, or as argument to defer.

benchmark                old ns/op    new ns/op    delta
BenchmarkSearchWrappers       1171          354  -69.77%
BenchmarkCallClosure             3            3  -12.54%
BenchmarkCallClosure1          119            7  -93.95%
BenchmarkCallClosure2          183           74  -59.18%
BenchmarkCallClosure3          187           75  -59.57%
BenchmarkCallClosure4          187           76  -58.98%

Compared to Go 1:
benchmark                  old ns/op    new ns/op    delta
BenchmarkSearchWrappers         3208          354  -88.97%

Fixes issue 3520.

[remyoudompheng, 2013-02-24 10:09:21]

Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),

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

[DMorsing, 2013-02-24 12:19:51]

LGTM

[bradfitz, 2013-02-24 16:54:52]

Nice. Could you include a Go test using testing.AllocsPerRun so this can't
silently regress?

On Sun, Feb 24, 2013 at 2:09 AM, <remyoudompheng@gmail.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> cmd/gc: apply escape analysis results to closures.
>
> This avoids an allocation when closures are used
> as "macros", in Walk idioms, or as argument to defer.
>
> benchmark                old ns/op    new ns/op    delta
> BenchmarkSearchWrappers       1171          354  -69.77%
> BenchmarkCallClosure             3            3  -12.54%
> BenchmarkCallClosure1          119            7  -93.95%
> BenchmarkCallClosure2          183           74  -59.18%
> BenchmarkCallClosure3          187           75  -59.57%
> BenchmarkCallClosure4          187           76  -58.98%
>
> Compared to Go 1:
> benchmark                  old ns/op    new ns/op    delta
> BenchmarkSearchWrappers         3208          354  -88.97%
>
> Fixes issue 3520.
>
> Please review this at
https://codereview.appspot.**com/7397056/<https://codereview.appspot.com/7397...
>
> Affected files:
>   M src/cmd/gc/closure.c
>   M src/pkg/sort/search_test.go
>
>
> Index: src/cmd/gc/closure.c
> ==============================**==============================**=======
> --- a/src/cmd/gc/closure.c
> +++ b/src/cmd/gc/closure.c
> @@ -241,14 +241,18 @@
>         }
>
>         clos = nod(OCOMPLIT, N, nod(OIND, typ, N));
> +       clos->esc = func->esc;
>         clos->right->implicit = 1;
>         clos->list = concat(list1(nod(OCFUNC, func->closure->nname, N)),
> func->enter);
>
>         // Force type conversion from *struct to the func type.
>         clos = nod(OCONVNOP, clos, N);
>         clos->type = func->type;
> -
> +
>         typecheck(&clos, Erv);
> +       // typecheck will insert a PTRLIT node under CONVNOP,
> +       // tag it with escape analysis result.
> +       clos->left->esc = func->esc;
>         walkexpr(&clos, init);
>
>         return clos;
> Index: src/pkg/sort/search_test.go
> ==============================**==============================**=======
> --- a/src/pkg/sort/search_test.go
> +++ b/src/pkg/sort/search_test.go
> @@ -117,6 +117,17 @@
>         }
>  }
>
> +func BenchmarkSearchWrappers(b *testing.B) {
> +       for i := 0; i < b.N; i++ {
> +               SearchInts(data, 11)
> +               SearchFloat64s(fdata, 2.1)
> +               SearchStrings(sdata, "")
> +               IntSlice(data).Search(0)
> +               Float64Slice(fdata).Search(2.**0)
> +               StringSlice(sdata).Search("x")
> +       }
> +}
> +
>  // Abstract exhaustive test: all sizes up to 100,
>  // all possible return values.  If there are any small
>  // corner cases, this test exercises them.
>
>
> --
>
> ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to
golang-dev+unsubscribe@**googlegroups.com<golang-dev%2Bunsubscribe@googlegrou...
> .
> For more options, visit
https://groups.google.com/**groups/opt_out<https://groups.google.com/groups/o...
> .
>
>
>

[minux1, 2013-02-24 16:56:12]

https://codereview.appspot.com/7397056/diff/5001/src/pkg/sort/search_test.go
File src/pkg/sort/search_test.go (right):

https://codereview.appspot.com/7397056/diff/5001/src/pkg/sort/search_test.go#...
src/pkg/sort/search_test.go:120: func BenchmarkSearchWrappers(b *testing.B) {
b.ReportAllocs() ?

[remyoudompheng, 2013-02-24 17:13:34]

Please take another look.

On 2013/02/24 16:54:52, bradfitz wrote:
> Nice. Could you include a Go test using testing.AllocsPerRun so this can't
> silently regress?

Done.

On 2013/02/24 16:56:12, minux wrote:
> https://codereview.appspot.com/7397056/diff/5001/src/pkg/sort/search_test.go
> File src/pkg/sort/search_test.go (right):
> 
>
https://codereview.appspot.com/7397056/diff/5001/src/pkg/sort/search_test.go#...
> src/pkg/sort/search_test.go:120: func BenchmarkSearchWrappers(b *testing.B) {
> b.ReportAllocs() ?

I think it would pollute the ouptut to print zeroes if there is already a test,
since people can use -benchmem if needed.

[bradfitz, 2013-02-24 17:17:14]

LGTM on Go part.

Thanks.

On Sun, Feb 24, 2013 at 9:13 AM, <remyoudompheng@gmail.com> wrote:

> Please take another look.
>
>
> On 2013/02/24 16:54:52, bradfitz wrote:
>
>> Nice. Could you include a Go test using testing.AllocsPerRun so this
>>
> can't
>
>> silently regress?
>>
>
> Done.
>
>
> On 2013/02/24 16:56:12, minux wrote:
>
> https://codereview.appspot.**com/7397056/diff/5001/src/pkg/**
>
sort/search_test.go<https://codereview.appspot.com/7397056/diff/5001/src/pkg/sort/search_test.go>
>
>> File src/pkg/sort/search_test.go (right):
>>
>
>
> https://codereview.appspot.**com/7397056/diff/5001/src/pkg/**
>
sort/search_test.go#newcode120<https://codereview.appspot.com/7397056/diff/5001/src/pkg/sort/search_test.go#newcode120>
>
>> src/pkg/sort/search_test.go:**120: func BenchmarkSearchWrappers(b
>>
> *testing.B) {
>
>> b.ReportAllocs() ?
>>
>
> I think it would pollute the ouptut to print zeroes if there is already
> a test, since people can use -benchmem if needed.
>
>
>
https://codereview.appspot.**com/7397056/<https://codereview.appspot.com/7397...
>

[DMorsing, 2013-02-24 17:17:24]

New patch set has chunk mismatch.

Didn't someone put in a check for that in the hg util a while back?

[remyoudompheng, 2013-02-24 21:39:31]

On 2013/02/24 17:17:24, DMorsing wrote:
> New patch set has chunk mismatch.

Reuploaded.

[dave_cheney.net, 2013-02-24 21:52:57]

I checked the Rietveld issue tracker a few weeks back but found no issue
relating to this. Is anyone aware of an issue that the Rietveld folks are
working on, or should I raise one ?

On 25/02/2013, at 4:17, daniel.morsing@gmail.com wrote:

> New patch set has chunk mismatch.
> 
> Didn't someone put in a check for that in the hg util a while back?
> 
> https://codereview.appspot.com/7397056/
> 
> -- 
> 
> ---You received this message because you are subscribed to the Google Groups
"golang-dev" group.
> To unsubscribe from this group and stop receiving emails from it, send an
email to golang-dev+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/groups/opt_out.
> 
> 

[bradfitz, 2013-02-24 22:54:36]

Folks work on Rietveld?
 On Feb 24, 2013 1:52 PM, "Dave Cheney" <dave@cheney.net> wrote:

> I checked the Rietveld issue tracker a few weeks back but found no issue
> relating to this. Is anyone aware of an issue that the Rietveld folks are
> working on, or should I raise one ?
>
> On 25/02/2013, at 4:17, daniel.morsing@gmail.com wrote:
>
> > New patch set has chunk mismatch.
> >
> > Didn't someone put in a check for that in the hg util a while back?
> >
> > https://codereview.appspot.com/7397056/
> >
> > --
> >
> > ---You received this message because you are subscribed to the Google
> Groups "golang-dev" group.
> > To unsubscribe from this group and stop receiving emails from it, send
> an email to golang-dev+unsubscribe@googlegroups.com.
> > For more options, visit https://groups.google.com/groups/opt_out.
> >
> >
>

[rsc, 2013-02-25 16:31:42]

Yes. folks do work on Rietveld, mainly people from the Go and Chromium projects.
You can mail codereview-discuss@ if you want to ask if anyone has any ideas. You
can also mail code reviews there, but CC me too.

Thanks.
Russ

[rsc, 2013-02-25 16:33:44]

LGTM

[remyoudompheng, 2013-02-25 23:40:41]

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

cmd/gc: apply escape analysis results to closures.

This avoids an allocation when closures are used
as "macros", in Walk idioms, or as argument to defer.

benchmark                old ns/op    new ns/op    delta
BenchmarkSearchWrappers       1171          354  -69.77%
BenchmarkCallClosure             3            3  -12.54%
BenchmarkCallClosure1          119            7  -93.95%
BenchmarkCallClosure2          183           74  -59.18%
BenchmarkCallClosure3          187           75  -59.57%
BenchmarkCallClosure4          187           76  -58.98%

Compared to Go 1:
benchmark                  old ns/op    new ns/op    delta
BenchmarkSearchWrappers         3208          354  -88.97%

Fixes issue 3520.

R=daniel.morsing, bradfitz, minux.ma, dave, rsc
CC=golang-dev
https://codereview.appspot.com/7397056