Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(247)

Issue 4954043: code review 4954043: gc: tweak and enable escape analysis (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
10 years, 4 months ago by rsc
Modified:
10 years, 4 months ago
Reviewers:
CC:
lvd, dfc, gustavo_niemeyer.net, golang-dev
Visibility:
Public.

Description

gc: tweak and enable escape analysis -s now means *disable* escape analysis. Fix escape leaks for struct/slice/map literals. Add ... tracking. Rewrite new(T) and slice literal into stack allocation when safe. Add annotations to reflect. Reflect is too chummy with the compiler, so changes like these affect it more than they should.

Patch Set 1 #

Patch Set 2 : diff -r 6577cd4b870f https://go.googlecode.com/hg #

Patch Set 3 : diff -r 6577cd4b870f https://go.googlecode.com/hg #

Patch Set 4 : diff -r 6577cd4b870f https://go.googlecode.com/hg #

Total comments: 3

Patch Set 5 : diff -r 29e5ff7664e2 https://go.googlecode.com/hg #

Patch Set 6 : diff -r 29e5ff7664e2 https://go.googlecode.com/hg #

Patch Set 7 : diff -r 29e5ff7664e2 https://go.googlecode.com/hg #

Patch Set 8 : diff -r 29e5ff7664e2 https://go.googlecode.com/hg #

Total comments: 2

Patch Set 9 : diff -r 7fec8679f10d https://go.googlecode.com/hg #

Patch Set 10 : diff -r 5e1053337103 https://go.googlecode.com/hg/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+567 lines, -321 lines) Patch
M src/cmd/gc/doc.go View 1 2 3 1 chunk +0 lines, -2 lines 0 comments Download
M src/cmd/gc/esc.c View 1 2 3 4 5 6 7 21 chunks +166 lines, -73 lines 0 comments Download
M src/cmd/gc/gen.c View 1 2 3 4 3 chunks +3 lines, -5 lines 0 comments Download
M src/cmd/gc/go.h View 1 2 3 4 5 6 5 chunks +30 lines, -18 lines 0 comments Download
M src/cmd/gc/lex.c View 1 2 3 4 2 chunks +7 lines, -3 lines 0 comments Download
M src/cmd/gc/sinit.c View 1 2 3 4 2 chunks +9 lines, -4 lines 0 comments Download
M src/cmd/gc/subr.c View 1 2 3 4 5 6 2 chunks +11 lines, -0 lines 0 comments Download
M src/cmd/gc/typecheck.c View 1 2 3 4 2 chunks +2 lines, -2 lines 0 comments Download
M src/cmd/gc/walk.c View 1 2 3 4 9 chunks +36 lines, -17 lines 0 comments Download
M src/pkg/reflect/value.go View 1 2 chunks +31 lines, -0 lines 0 comments Download
M test/escape2.go View 1 2 3 4 5 6 12 chunks +272 lines, -197 lines 0 comments Download

Messages

Total messages: 24
rsc
Hello lvd (cc: golang-dev@googlegroups.com), I'd like you to review this change to https://go.googlecode.com/hg
10 years, 4 months ago (2011-08-25 19:31:14 UTC) #1
rsc
in fmt: $ gotest rm -f _test/fmt.a 6g -o _gotest_.6 doc.go format.go print.go scan.go rm ...
10 years, 4 months ago (2011-08-25 19:43:20 UTC) #2
dfc
That it so awesome. Sent from my iPhone On 26/08/2011, at 5:43, Russ Cox <rsc@golang.org> ...
10 years, 4 months ago (2011-08-26 05:18:21 UTC) #3
lvd
very nice. except the s/floodgen/walkgen., but have it your way. tested on exp/go/eval too i ...
10 years, 4 months ago (2011-08-26 06:19:29 UTC) #4
lvd
On 2011/08/26 06:19:29, lvd wrote: > very nice. except the s/floodgen/walkgen., but have it your ...
10 years, 4 months ago (2011-08-26 06:23:16 UTC) #5
lvd
On 2011/08/26 06:23:16, lvd wrote: > On 2011/08/26 06:19:29, lvd wrote: > > very nice. ...
10 years, 4 months ago (2011-08-26 09:56:34 UTC) #6
lvd
http://codereview.appspot.com/4954043/diff/7001/test/escape2.go File test/escape2.go (right): http://codereview.appspot.com/4954043/diff/7001/test/escape2.go#newcode746 test/escape2.go:746: m := &Bar{ii: x} // ERROR "&struct literal escapes ...
10 years, 4 months ago (2011-08-26 11:17:09 UTC) #7
rsc
thanks for trying it out. i will find a copy of exp/eval and run it. ...
10 years, 4 months ago (2011-08-26 13:16:05 UTC) #8
rsc
fixed escape2 + uploaded. (comment was wrong). exp/eval next
10 years, 4 months ago (2011-08-26 15:20:11 UTC) #9
rsc
> http://codereview.appspot.com/4954043/diff/7001/src/cmd/gc/go.h#oldcode294 > src/cmd/gc/go.h:294: int escfloodgen; // increased for every flood to detect > loops ...
10 years, 4 months ago (2011-08-26 18:57:06 UTC) #10
rsc
PTAL I found the exp/eval bug - I was missing else blocks. To fix that ...
10 years, 4 months ago (2011-08-26 19:08:27 UTC) #11
lvd
happiness. mostly. i guess we'll just not agree on the walkgen thing, but it's not ...
10 years, 4 months ago (2011-08-27 12:23:09 UTC) #12
gustavo_niemeyer.net
Just as an empirical data point, the same benchmark with mgo that got a speed ...
10 years, 4 months ago (2011-08-27 21:29:32 UTC) #13
lvd
On Sat, Aug 27, 2011 at 23:29, Gustavo Niemeyer <gustavo@niemeyer.net>wrote: > Just as an empirical ...
10 years, 4 months ago (2011-08-27 21:50:38 UTC) #14
gustavo_niemeyer.net
> that is very strange. when you say enabled, do you mean /with/ -s after ...
10 years, 4 months ago (2011-08-27 21:56:30 UTC) #15
gustavo_niemeyer.net
> Enabled is this CL without changes and disabled is this CL with > debug['s']++ ...
10 years, 4 months ago (2011-08-27 23:07:19 UTC) #16
gustavo_niemeyer.net
> Unsurprisingly, the cost seems to come from stack handling. Without > this CL, the ...
10 years, 4 months ago (2011-08-27 23:14:59 UTC) #17
rsc
Not worried about stack size for this CL. Playing games with stack size to help ...
10 years, 4 months ago (2011-08-28 02:22:51 UTC) #18
gustavo_niemeyer.net
> Not worried about stack size for this CL. > > Playing games with stack ...
10 years, 4 months ago (2011-08-28 03:34:18 UTC) #19
rsc
On Sat, Aug 27, 2011 at 23:33, Gustavo Niemeyer <gustavo@niemeyer.net> wrote: >> Playing games with ...
10 years, 4 months ago (2011-08-28 15:41:48 UTC) #20
rsc
On Sat, Aug 27, 2011 at 08:23, <lvd@google.com> wrote: > instead of a separate list, ...
10 years, 4 months ago (2011-08-28 15:48:59 UTC) #21
rsc
*** Submitted as http://code.google.com/p/go/source/detail?r=8bf8b897037f *** gc: tweak and enable escape analysis -s now means *disable* ...
10 years, 4 months ago (2011-08-28 16:05:06 UTC) #22
gustavo_niemeyer.net
Thanks for the explanation Russ. It's appreciated. > It's not that simple. Your explanation makes ...
10 years, 4 months ago (2011-08-28 18:19:51 UTC) #23
rsc
10 years, 4 months ago (2011-08-29 01:50:54 UTC) #24
On Sun, Aug 28, 2011 at 14:19, Gustavo Niemeyer <gustavo@niemeyer.net> wrote:
> Understood the explanation and agree.  As a detail, the overall
> problem description makes it sound like it's merely a matter of
> positioning, and seems to ignore the fact that escape analysis is
> indeed increasing the frame size in some cases, which means a
> shallower depth of calls kicks the segmentation logic.  This is of
> course more relevant in cases such as marshalling and unmarshalling
> that depend on recursing up and down more heavily. Is that a valid
> statement?

Yes but only with enough caveats.  For a heavily recursive
function, something like go/parser, it's zipping up and down
the stack across many segments like there's no tomorrow.
If it can only fit 0.9n frames into a segment where before it
could fit n frames, then it would see just a 1.1x slowdown
caused by stack splits.

To see the 100x that you are seeing, the most likely story is
that before you never did any stack splits, and now the
processing of leaves in your recursive function is typically
on another segment, so each leaf call is doing a stack split.
Before you might have needed 0.95 frames and now you
need 1.1 frames.  Even so I am a little surprised that the
smaller load on the garbage collector does not make up
for the extra frame sizes.

If nothing else this is a good thing for profiling to record.
I created issue 2197.

Russ
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b