Skip to content
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

Jump to bottom

cmd/compile: merge temporaries more aggressively #8740

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

cmd/compile: merge temporaries more aggressively #8740

josharian opened this issue Sep 15, 2014 · 9 comments
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone
Unplanned

Comments

@josharian
Copy link
Contributor 

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.
@bradfitz bradfitz modified the milestone: Go1.5 Dec 16, 2014
@josharian
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor

rsc commented May 19, 2015

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

@josharian
Copy link
Contributor Author

CL 10251

@gopherbot
Copy link

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

@josharian josharian mentioned this issue May 20, 2015
Open
josharian added a commit to josharian/go that referenced this issue May 26, 2015
@josharian
cmd/internal/gc: merge temporaries more aggressively
6086ce7 
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 cmd/gc: merge temporaries more aggressively 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
Copy link
Contributor

/cc @randall77

@josharian
Copy link
Contributor Author

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 The path to resolution is known, but the work has not been done. label Oct 6, 2016
@quentinmit
Copy link
Contributor

@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
Copy link
Contributor Author

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.

@geovanisouza92 geovanisouza92 mentioned this issue Jul 19, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees
No one assigned
Labels
NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
7 participants
@bradfitz @josharian @rsc @quentinmit @dvyukov @randall77 @gopherbot