New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

cmd/compile: merge temporaries more aggressively #8740

Open
josharian opened this Issue Sep 15, 2014 · 9 comments

Comments

Projects
None yet
7 participants
@josharian
Contributor

josharian commented Sep 15, 2014

CL 12829043 added a compiler pass that merges temporary variables with equal types and
non-overlapping lifetimes.

I believe that "equal types" is sufficient but not necessary and that
"equivalent w.r.t. GC" is both necessary and sufficient. (Please correct me if
I am wrong!) This should allow more temporaries to be merged, further reducing stack
usage.

@josharian josharian added new labels Sep 15, 2014

@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014

@josharian

This comment has been minimized.

Contributor

josharian commented Feb 6, 2015

I took a quick hack at this. It does appear to offer some nice stack use wins, but it breaks our dwarf generation. Also, twobitwalktype1 is a compiler performance bottleneck. This will need to wait at the least until the compiler transition to Go is completed.

My ugly hack code: https://github.com/josharian/go/compare/issue-8740. From the commit message there:

In godoc binary

Top 20 changed frame sizes, by absolute change

2152    1304    -848    -39.4%  golang.org/x/tools/godoc/analysis       "".(*analysis).doCallgraph 
2496    1976    -520    -20.8%  golang.org/x/tools/godoc/analysis       "".Run 
952     448     -504    -52.9%  html/template   "".(*escaper).escapeListConditionally 
1408    920     -488    -34.7%  golang.org/x/tools/go/ssa       "".(*builder).stmt 
1680    1192    -488    -29.0%  golang.org/x/tools/go/loader    "".(*Config).Load 
2160    1688    -472    -21.9%  golang.org/x/tools/go/types     "".(*Checker).collectObjects 
1904    1472    -432    -22.7%  golang.org/x/tools/go/ssa       "".(*builder).expr0 
728     320     -408    -56.0%  html/template   "".(*escaper).commit 
552     168     -384    -69.6%  encoding/gob    "".registerBasics 
1608    1232    -376    -23.4%  crypto/x509     "".buildExtensions 
1208    840     -368    -30.5%  go/doc  "".playExample 
1792    1424    -368    -20.5%  crypto/x509     "".parseCertificate 
1112    760     -352    -31.7%  golang.org/x/tools/godoc        "".(*handlerServer).GetPageInfo 
1384    1032    -352    -25.4%  golang.org/x/tools/go/ssa       "".(*Program).CreateTestMainPackage 
1792    1440    -352    -19.6%  golang.org/x/tools/go/types     "".(*Checker).builtin 
1968    1616    -352    -17.9%  golang.org/x/tools/go/types     "".(*Checker).stmt 
1160    824     -336    -29.0%  golang.org/x/tools/go/ssa       "".(*Package).Build 
1680    1368    -312    -18.6%  golang.org/x/tools/go/types     "".(*Checker).exprInternal 
2384    2088    -296    -12.4%  go/build        "".(*Context).Import 
1848    1568    -280    -15.2%  golang.org/x/tools/godoc/analysis       "".(*analysis).doChannelPeers 

Top 20 changed frame sizes, by percent change

552     168     -384    -69.6%  encoding/gob    "".registerBasics 
168     72      -96     -57.1%  archive/zip     type..eq."".checksumReader dupok 
728     320     -408    -56.0%  html/template   "".(*escaper).commit 
952     448     -504    -52.9%  html/template   "".(*escaper).escapeListConditionally 
144     72      -72     -50.0%  text/template   "".sortKeys 
200     104     -96     -48.0%  net/http        type..eq."".conn dupok 
136     72      -64     -47.1%  archive/zip     type..eq."".fileWriter dupok 
136     72      -64     -47.1%  crypto/cipher   type..eq."".StreamWriter dupok 
136     72      -64     -47.1%  golang.org/x/tools/go/ssa       type..eq."".DebugRef dupok 
136     72      -64     -47.1%  golang.org/x/tools/go/types     type..eq."".operand dupok 
336     184     -152    -45.2%  encoding/gob    "".init 
120     72      -48     -40.0%  compress/flate  "".(*compressor).initDeflate 
2152    1304    -848    -39.4%  golang.org/x/tools/godoc/analysis       "".(*analysis).doCallgraph 
128     80      -48     -37.5%  go/doc  "".(*Package).Filter 
64      40      -24     -37.5%  compress/flate  "".(*huffmanBitWriter).writeCode 
512     328     -184    -35.9%  golang.org/x/tools/go/ast/astutil       "".AddNamedImport 
1408    920     -488    -34.7%  golang.org/x/tools/go/ssa       "".(*builder).stmt 
232     152     -80     -34.5%  net/http        "".init 
560     368     -192    -34.3%  text/template   "".(*Template).Clone 
120     80      -40     -33.3%  go/parser       "".(*parser).tryIdentOrType 
@dvyukov

This comment has been minimized.

Member

dvyukov commented Feb 7, 2015

Nice!
This can significantly reduce memory consumption and frequency of GCs for servers with lots of goroutines.

@rsc rsc removed the repo-main label Apr 14, 2015

@rsc

This comment has been minimized.

Contributor

rsc commented May 19, 2015

The compare link doesn't seem to work anymore. Can you refresh it somehow?

@josharian

This comment has been minimized.

Contributor

josharian commented May 19, 2015

CL 10251

@gopherbot

This comment has been minimized.

gopherbot commented May 19, 2015

CL https://golang.org/cl/10251 mentions this issue.

josharian added a commit to josharian/go that referenced this issue May 26, 2015

cmd/internal/gc: merge temporaries more aggressively
DO NOT SUBMIT

* I think that we may be able to merge even more.
* Needs updated frame size numbers.
* I don't understand the impact this might have
  on our generated DWARF.
* This feels like a high risk change relative
  for Go 1.5.
* Confirm that calling onebitwalktype1 doesn't
  have any negative compiler performance impact.

Fixes golang#8740.

Change-Id: I551b6328e66660953c9a569a461d945071799248

@josharian josharian modified the milestones: Go1.6, Go1.5 Jun 6, 2015

@rsc rsc changed the title from cmd/gc: merge temporaries more aggressively to cmd/compile: merge temporaries more aggressively Jun 8, 2015

@rsc rsc modified the milestones: Unplanned, Go1.6 Dec 5, 2015

@bradfitz bradfitz modified the milestones: Go1.8Maybe, Unplanned May 10, 2016

@bradfitz

This comment has been minimized.

Member

bradfitz commented May 10, 2016

@josharian

This comment has been minimized.

Contributor

josharian commented May 23, 2016

I have a prototype of this for SSA, although it needs some cleanup. It still helps some, although not as dramatically as before SSA. I plan to finish it up and mail it early in the Go 1.8 cycle.

@josharian josharian modified the milestones: Go1.8Early, Go1.8Maybe May 23, 2016

@josharian josharian self-assigned this May 23, 2016

@quentinmit quentinmit added the NeedsFix label Oct 6, 2016

@quentinmit

This comment has been minimized.

Contributor

quentinmit commented Oct 6, 2016

@josharian Have you had a chance to look at this yet for 1.8?

@rsc rsc modified the milestones: Go1.8Early, Unplanned Oct 17, 2016

@josharian

This comment has been minimized.

Contributor

josharian commented Oct 24, 2016

I did. My recollection (now fuzzy) is that it helped but not enough to inspire me to finish re-implementing it.

I'll also admit to being a bit confused at the moment about the relationship between the stackalloc pass and the temporary merging code found in pgen.go. I've implemented this independently in both places, but never tried applying it to both at the same time.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment