Planning Go 1.13

[Andrew Bonventre]

Now that Go 1.12rc1 is out, the Go repos have been branched ("release-branch.go1.12") so the Go 1.13 development tree will open soon on the "master" branches.

This week the tree will open only for significant changes that need to be made early in the cycle, in relative quiet. Right now, these changes include the Go 2 number literals, error values, and signed shift counts. If anyone has any other large changes they would like to see land during this time, please suggest them by replying to this thread.

The tree will NOT be opened for general development until the final release is out around two weeks from today (the week of Feb 25). The schedule may change if there are further delays to the final release of 1.12.

We understand that this cuts the development cycle of 1.13 short, but we would rather not put more work on the core team to handle the influx of new changes while concurrently attempting to get the release out. This is also in line with our release cycle document, which states “if a release is delayed, so is work on the next release.”

One contributing factor for the release delay is significant changes landing too late into the freeze. The freeze is meant to slow down the rate of change and be a time to test for any as-yet-undiscovered high-priority bugs that must be fixed in this release cycle. In the past we’ve said that changes mailed before the freeze can be reviewed and landed during the freeze. Unfortunately, too many significant changes are mailed right before the freeze. Those end up taking significant time to review and having a high risk of introducing new bugs. For Go 1.13, changes that are not yet reviewed by two weeks into the freeze, or that are simply too large or risky, will be postponed to the next cycle. If you want a change to land in Go 1.13, don’t wait until the end of the freeze: please mail it as early as you can, and ping reviewers if the review seems stalled.

With that said, let's use this thread to discuss people's plans for Go 1.13.

(These are things you *PLAN TO DO YOURSELF*, not things you want other people to do.)

[Matthew Dempsky]

I have two changes planned:

1. Rewrite f(g()) for multi-value g() into "t0, t1, ... := g(); f(t0, t1, ...)" during type checking, to simplify later compiler passes (and fix latent issues). CL at https://go-review.googlesource.com/c/go/+/153841/. I expect this to be straightforward.

2. Rewrite escape analysis. I've documented my progress at https://github.com/golang/go/issues/23109 and I have a working rewrite at https://github.com/mdempsky/go/blob/esc5/src/cmd/compile/internal/gc/esc2.go that's about half the size of esc.go. This might be a bit messier, unfortunately, because the new code both better matches Go implementation details (so escape analysis tags differ in some cases, ultimately resulting in some objects changing whether they're stack- or heap-allocated), and it's also much simpler (so some of the extra debugging messages that many regress tests are written to expect don't make sense).

--
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+...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

[Russ Cox]

[Forked from planning thread.]

On Mon, Feb 11, 2019 at 7:18 PM 'Matthew Dempsky' via golang-dev <golan...@googlegroups.com> wrote:

2. Rewrite escape analysis. I've documented my progress at https://github.com/golang/go/issues/23109 and I have a working rewrite at https://github.com/mdempsky/go/blob/esc5/src/cmd/compile/internal/gc/esc2.go that's about half the size of esc.go. This might be a bit messier, unfortunately, because the new code both better matches Go implementation details (so escape analysis tags differ in some cases, ultimately resulting in some objects changing whether they're stack- or heap-allocated), and it's also much simpler (so some of the extra debugging messages that many regress tests are written to expect don't make sense).

Hi Matthew,

Thanks very much for working on cleaning this up.

Do you have a sense of how many stack allocations we are going to lose? I am very happy with how good the escape analysis has gotten at keeping things on the stack and it would be good not to regress. Of course, more things staying on the stack is always welcome. :-)

Thanks.

Russ

[Matthew Dempsky]

On Mon, Feb 11, 2019 at 5:48 PM Russ Cox <r...@golang.org> wrote:

Do you have a sense of how many stack allocations we are going to lose?

From memory, very few. I was manually spot checking all of the differences in the standard library, and overall my impression was the new implementation was marginally better. For example, there are a handful of "BAD: xxx should not escape" instances in the regress suite (e.g., cases like #7714) that my rewrite happens to fix because of more precise data flow.

The only notable regression I can recall is that parseSANExtension's return parameters (dnsNames, emailAddresses, etc) now heap allocate. This is because of how the escape analysis algorithm models indirect function calls and closures, and while I understand why the new code fails to stack allocate them, I couldn't explain why the old code succeeds.

I am very happy with how good the escape analysis has gotten at keeping things on the stack and it would be good not to regress.

My rewrite uses the same general algorithm as the existing implementation. It's just written much more simply and precisely (the latter being made easier thanks to the former).

[Matthew Dempsky]

More precise data. When building std + cmd at master (e871981b):

My implementation marks 64 additional expressions as "noescape" (that is, values that are dead immediately after their containing statement). It also marks every every "noescape" expression already marked by esc.go.

My implementation marks 124 variables/expressions that can be stack allocated, but that esc.go currently heap allocates.

My implementation heap allocates these 11 variables, that esc.go currently stack allocates:

regexp/regexp.go:617:2: srepl

net/nss.go:137:31: c

crypto/x509/verify.go:587:2: comparisonCount

crypto/x509/x509.go:1117:39: dnsNames

crypto/x509/x509.go:1117:49: emailAddresses

crypto/x509/x509.go:1117:74: ipAddresses

crypto/x509/x509.go:1117:96: uris

cmd/go/internal/modfetch/cache.go:218:29: rev

cmd/go/internal/modload/import.go:211:59: dir

cmd/go/internal/modload/import.go:230:7: d

cmd/compile/internal/gc/typecheck.go:359:17: n

That is, it more aggressively optimizes ~188 instances (I just noticed there's at least some overlap), but more pessimistically optimizes 11.

[Matthew Dempsky]

On Mon, Feb 11, 2019 at 7:42 PM Matthew Dempsky <mdem...@google.com> wrote:

regexp/regexp.go:617:2: srepl

net/nss.go:137:31: c

crypto/x509/verify.go:587:2: comparisonCount

crypto/x509/x509.go:1117:39: dnsNames

crypto/x509/x509.go:1117:49: emailAddresses

crypto/x509/x509.go:1117:74: ipAddresses

crypto/x509/x509.go:1117:96: uris

cmd/go/internal/modfetch/cache.go:218:29: rev

cmd/go/internal/modload/import.go:211:59: dir

cmd/go/internal/modload/import.go:230:7: d

cmd/compile/internal/gc/typecheck.go:359:17: n

These are all the same root cause (the last one is trickier to spot: the function literal at typecheck.go:1368 captures n by reference), so let me go ahead and elaborate on what that is.

Escape analysis fundamentally models the Go source as a bunch of abstract locations (e.g., x, y, z), and assignments between them (e.g., x = &y, x = y, x = *y, x = **y, ...).

An indirect call like "x = f()" is modeled as "x = *f" because f could potentially return any value stored in its closure. (It could also return any value that already escaped to the heap, but those aren't interesting.)

A function literal like "func() error { y++; return nil }" needs to store a reference to y in its closure. This needs to be modeled as "closure = &y", because if the closure leaks to the heap, then y also needs to leak to the heap.

Combining these two rules introduces a false data flow where an indirect call to that function literal infers that it could return &y. And if x ends up leaking (most commonly via a return parameter), then we have to heap allocate y. For example:

    //go:noinline

    func g(f func() error) error {

        return f()

    }

    func H() error {

        var y int

        return g(func() error {

            y++

            return nil

        })

    }

What's unclear to me is how the current escape analysis code avoids this problem. As far as I can tell, it's using these same rules.

Put differently, escape analysis processes the above code the same as:

type Closure struct {

yp  *int

err error

}

//go:noinline

func g(f *Closure) error {

return f.err

}

func F() error {

var y int

return g(&Closure{

yp:  &y,

err: nil,

})

}

We model "return f.err" as just "return *f", so when analyzing F, we can't tell if it might return &y.

Notably, the existing code esc.go and my rewrite both (pessimistically) heap allocate y here. This makes me suspect esc.go has a subtle issue where it's not actually handling indirect function calls / closures correctly somehow, and it's just getting lucky that some common cases are handled better.

[David Chase]

I am willing to believe the "just getting lucky" hypothesis; the closure code made me a little nervous, but I never managed to construct a counterexample.

[Matthew Dempsky]

I just figured it out. It's definitely a getting lucky thing.

So working from this example again:

    //go:noinline

    func g(f func() error) error {

        return f()

    }

    func H() error {

        var y int

        return g(func() error {

            y++

            return nil

        })

    }

There are a few relevant details about how esc.go handles this code:

1. It first analyzes g and summarizes it as "~r0 = *f" (using EscReturnBits in the esc tags). This is fine.

2. At L1064, we associate "H.func1 = &y" because y is captured by reference.

3. At L1269, there's code in escassign to rewrite "dst = H.func1" into "dst = &H.func1", since function literals are actually a reference type.

4. The call to "g(H.func1)" gets handled by escassignfromtag, which rewrites to ".out0 = *H.func1" (based on the summary from step 1 above) and calls escassign. (See L1479.)

The problem is the interaction between 3 and 4: escassign doesn't recurse on src when src.Op == ODEREF, so ".out0 = *H.func1" is left as-is, rather than being rewritten to ".out0 = *&H.func1".

[Ian Davis]

I am planning to flesh out a proposal for progressive image decoding mentioned at the end of https://github.com/golang/go/issues/27830

I don't know how much time I will have to spend on it but my aim is to have the core model described and a sample implementation for least one image type before the end of the 1.13 cycle.

[Austin Clements]

I just wanted to say this is really awesome. Thank you for working on this.

I see you already have a big comment at the top of esc2.go explaining the key idea. I'd love to see a detailed description of the algorithm, such as how the distance function is updated over the CFG (particularly how loops are handled), how this turns into a function summary, and how a few nontrivial AST nodes affect things as a concrete example.

The escape code has never been very maintainable partly because it's so complex and partly because the lack of documentation means it takes reverse engineering the whole thing before you can even figure out what change to make. Your rewrite is going to help a ton with the former. While you've got it all paged in, it would be extra amazing to also tackle the latter. :)

--

[Bryan C. Mills]

I'll be working module-mode usability and robustness in cmd/go.

My main areas of focus for 1.13 will be:

• Enabling module mode by default, including fixing tests in std to work in module mode.
• Simplifying configuration and authentication for private repos.
• Supporting automatic use of (and updates to) the main module's vendor directory.
• Making version resolution more robust.

I expect to address a number of other cmd/go issues as well. (I'm going to try to keep the Assignee field up-to-date on the GitHub issues, so you can search for `assignee:bcmills milestone:Go1.13` for the current list at any given point.)

[Carlos Eduardo Seo]

 

I plan to make the compiler start generating new POWER9 instructions on ppc64/ppc64le. For that purpose, I’ll propose a method similar to GOARM to make sure this doesn’t break things for the current default (POWER8). I’m also planning some runtime performance optimizations for POWER9.

 

To test those, I’m also setting up a new POWER9 builder.

 

-- 

Carlos Eduardo Seo

Software Engineer - Linux on Power Toolchain

cs...@linux.vnet.ibm.com

--

[Devon H. O'Dell]

I was hoping for Fuchsia in 1.12, but it wasn't really in a place
where that could occur. I am really hoping that I can finally get
Fuchsia merged for 1.13 so that I don't have to maintain a separate
port anymore. There are a couple of high level points that probably
need addressing before I start sending patches:

* Some code needs reorganization. We have a large amount of
non-syscall-related stuff in syscall/zx and this was discussed to some
extent in the GOOS=fuchsia thread a couple years ago. I'm not of the
opinion that the package naming or location is appropriate, so most of
that needs to either live as separate packages, or in runtime.
* I need help bootstrapping / making builder images. I've been
talking off-and-on with Brad to try to understand this, but need some
additional assistance here.

Since our fork has been maintained separately for some years, I
suspect there will also be open questions about the CONTRIBUTORS file.
I don't suspect AUTHORS will need updating, but the development of the
tree is probably not such that we can just merge commits over. I
wonder if it is reasonable to just add everyone in our commit history
that is not already listed in CONTRIBUTORS?

I'd like to work with folks early to figure out how to best overcome
these issues so that there's a good shot of actually getting this done
finally.

Kind regards,

--dho

[Nigel Tao]

On Tue, Feb 12, 2019 at 8:05 PM Ian Davis <m...@iandavis.com> wrote:
> I am planning to flesh out a proposal for progressive image decoding mentioned at the end of https://github.com/golang/go/issues/27830

As I said in https://github.com/golang/go/issues/27830#issuecomment-427208514,
I don't think progressive image decoding needs to be tied to the
stdlib and its 6-monthly release cycle. Even in the long term, I
suspect that it might best live outside of the standard library.

[CHIGOT, CLEMENT]

For 1.13, I plan to finalise aix/ppc64 port: 

 - fix the last remaining bugs

 - port cgo (which is almost done locally btw) 

 - port golang.org/x packages 

 - port pprof 

 - port remaining linker/compiler modes if I still have time. 

Sincerely

Clément Chigot

ATOS Bull SAS

1 rue de Provence - 38432 Échirolles - France

---

From: golan...@googlegroups.com <golan...@googlegroups.com> on behalf of Nigel Tao <nige...@golang.org>
Sent: Wednesday, February 13, 2019 1:32:47 AM
To: Ian Davis
Cc: golang-dev
Subject: Re: [golang-dev] Planning Go 1.13

 

On Tue, Feb 12, 2019 at 8:05 PM Ian Davis <m...@iandavis.com> wrote:

> I am planning to flesh out a proposal for progressive image decoding mentioned at the end of https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgolang%2Fgo%2Fissues%2F27830&amp;data=02%7C01%7Cclement.chigot%40atos.net%7Cc54cfe36abb2499fcd7108d6914ae1f7%7C33440fc6b7c7412cbb730e70b0198d5a%7C0%7C1%7C636856148138143069&amp;sdata=gReR%2FohXr4UQp1zuEMTWDmKz2MRGhA6wlvfEv%2FLS0Hg%3D&amp;reserved=0

As I said in https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgithub.com%2Fgolang%2Fgo%2Fissues%2F27830%23issuecomment-427208514&amp;data=02%7C01%7Cclement.chigot%40atos.net%7Cc54cfe36abb2499fcd7108d6914ae1f7%7C33440fc6b7c7412cbb730e70b0198d5a%7C0%7C1%7C636856148138143069&amp;sdata=1%2BHvBOT6hvK972E%2FjSFurj1kQ6UQ7wrUQLsnHdFg7Xc%3D&amp;reserved=0,

I don't think progressive image decoding needs to be tied to the
stdlib and its 6-monthly release cycle. Even in the long term, I
suspect that it might best live outside of the standard library.

--
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+...@googlegroups.com.

For more options, visit https://emea01.safelinks.protection.outlook.com/?url=https%3A%2F%2Fgroups.google.com%2Fd%2Foptout&amp;data=02%7C01%7Cclement.chigot%40atos.net%7Cc54cfe36abb2499fcd7108d6914ae1f7%7C33440fc6b7c7412cbb730e70b0198d5a%7C0%7C1%7C636856148138143069&amp;sdata=XHQJTbWkmDa9mqs2iudNsH8Sw%2FL%2BXS1rBbOMgfUQnAU%3D&amp;reserved=0.

[Zach Jones]

I plan to make improvements to bounds-check elimination via the compiler’s prove pass.

There seem to be quite a few issues targeting BCE on the issue tracker. I’m currently wading through them, identifying duplicates and loosely categorising what remains.

If you find unnecessary bounds checks remaining in std or cmd , especially in hot code paths, let me know (and/or post an issue).

-Zach

[Makxsadbek Akxmedov]

I am planning the following change: net: Make it possible to determine if a lookup error is errNoSuchHost

Add a new `IsNoSuchHost` field to `net.DNSError` struct to determine if lookup error is errNoSuchHost.

This can be achieve w/o adding a new field. I am ready to discuss and open to suggestions.

I found this issue easy for the first contribution.

[Daniel Martí]

In 1.12, I worked a bit on encoding/json's encoding and decoding
performance. I intend to continue on that for 1.13, particularly on the
decoder side, which is the slowest. See https://golang.org/issues/28923.

I have similar speed-up CLs for other packages like base64, base32, hex,
and pem. Though those are low-hanging fruit, and not as complex as the
json CLs.

I also intend to work on the init speed of cmd/go. Lots of programs and
scripts run the binary, including the recent go/packages. So an extra
couple of milliseconds to initialise the main package do add up quickly.
See https://golang.org/issues/29382.

I'll add os.UserConfigDir and token.IsIdentifier, which come from
proposals of mine accepted during the freeze.

Finally, I'm helping Zach (github.com/zdjones) get started with the
compiler, and particularly its prove pass. He's doing well on his own
thanks to the READMEs, but I've filed https://golang.org/issues/30074 as
I noticed many of the well-known compiler dev tips weren't written down.

I've also encouraged him to write a README section for the prove pass,
as I'd like to understand how it works :)

No other plans for 1.13, for now. If you know of another decoder or init
function which is too slow, feel free to ping me about it.

[David Chase]

CLs that did not make the cut for 1.12, that might be useful for 1.13:

https://go-review.googlesource.com/c/go/+/136375 cmd/compile: enhance induction variable detection for unrolled loops

https://go-review.googlesource.com/c/go/+/148777 cmd/compile: enable inline of functions containing "for" loops

https://go-review.googlesource.com/c/go/+/37338 cmd/compile: hoist invariants out of loops

I was not necessarily planning to work on these, I will instead be working on debugging-related things (I think).

--
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+...@googlegroups.com.

For more options, visit https://groups.google.com/d/optout.

[Russ Cox]

On Mon, Feb 11, 2019 at 10:42 PM Matthew Dempsky <mdem...@google.com> wrote:

My implementation heap allocates these 11 variables, that esc.go currently stack allocates:

regexp/regexp.go:617:2: srepl

net/nss.go:137:31: c

crypto/x509/verify.go:587:2: comparisonCount

crypto/x509/x509.go:1117:39: dnsNames

crypto/x509/x509.go:1117:49: emailAddresses

crypto/x509/x509.go:1117:74: ipAddresses

crypto/x509/x509.go:1117:96: uris

cmd/go/internal/modfetch/cache.go:218:29: rev

cmd/go/internal/modload/import.go:211:59: dir

cmd/go/internal/modload/import.go:230:7: d

cmd/compile/internal/gc/typecheck.go:359:17: n

There seem to be two relevant forms. 

I second Austin's comments about the rewrite being fantastic.

But both of these cases seem worth preserving

if at all possible, and it does look possible to me.

They are common, simple uses of closures.

The first bunch mainly boil down to:

func call(f func()) { f() }
func g() {var x int; call(func() { x = 1 })}

That is, most of them are assigning to the captured variable

from within a closure that is known not to escape

(that is, the closure will not be called after the function 

in which it is created returns).

For example, in regexp:

srepl := ""

b := re.replaceAll(src, "", n, func(dst []byte, match []int) []byte {

if len(srepl) != len(repl) {

srepl = string(repl)

}

return re.expand(dst, srepl, src, "", match)

})

Clearly the compile can tell that the closure does not

escape; it is only called from replaceAll, not retained.

It would be unfortunate indeed to make every such piece

of code start forcing assigned captured variables onto the

heap.

Originally, closures always stored addresses of referenced

variables. At some point an optimization was added that

captured variables that are not later modified by the outer

function have their values, not their addresses, recorded

in the closure. If not for this optimization, I suspect the same

problems that force heap allocation above would force

heap allocation even for:

func call(f func() int) { f() }
func g() {var x int; call(func() int { return x })}

Obviously that would not be good either. And it doesn't

happen, because of another optimization. But the escape

analysis should be able to make the right call on its own too.

If we tweak that example to modify x after the closure creation,

that will disable the closure-value optimization:

func call(f func() int) { f() }
func g() {var x int; call(func() int { return x }); x++}

Then the closure must record the address of x,

and I bet that would fall back into the same problem

as with the assignments, thinking that &x escapes.

In fact that seems to be what is happening in the

last couple cases. For example the cmd/go code

boils down to:

for d := dir; d != mdir && len(d) > len(mdir); {

haveGoModCache.Do(d, func() interface{} { use(d) })
d = ...

}

The cmd/gc typecheck1 example is much larger but

boils down to the same thing. The variable n is reassigned

in various places in the outer function, so the closure

func() { ... fmt.Sprintf(... n ...) } conservative captures

n's address instead of its value.

For what it's worth, a bit of the history of that function.

In the original C code and in the initial conversion from C to Go,

typecheck1 took a parameter of type **Node and rewrote it in

place instead of returning the new value. The escape analysis

could not see that the argument did not escape (because of the recursion),

so all calls like typecheck1(&l) were forced to allocate l on the heap.

This was one of many new allocations compared to the C code

that we worked to remove to improve performance, in this case

by making callers write l = typecheck1(l) instead. If the first thing

typecheck1 does is make a heap-allocated copy of its argument,

because &n is over-conservatively seen as escaping, then that's

a regression back to the original allocation pattern.

(The regression is worse, since some of calls to to the original 

typecheck1 were passing the address of heap values already

and so did not trigger additional allocation; with the new escape

analysis, every call will allocate.)

Maybe the current compiler or GC makes this particular allocation

unimportant, but it's still a likely regression in similar examples

in other programs.

On Mon, Feb 11, 2019 at 11:52 PM Matthew Dempsky <mdem...@google.com> wrote:

Escape analysis fundamentally models the Go source as a bunch of abstract locations (e.g., x, y, z), and assignments between them (e.g., x = &y, x = y, x = *y, x = **y, ...).

An indirect call like "x = f()" is modeled as "x = *f" because f could potentially return any value stored in its closure. (It could also return any value that already escaped to the heap, but those aren't interesting.)

A function literal like "func() error { y++; return nil }" needs to store a reference to y in its closure. This needs to be modeled as "closure = &y", because if the closure leaks to the heap, then y also needs to leak to the heap.

What esc.go does is a bit more subtle than this.

The linking of the captured variables to the closure at :1064

makes sure that if the closure itself escapes, then the

addresses of the variables are understood to escape too.

But in these examples the closure itself does not escape.

When the closure itself does not escape, the analysis

models the code inside the closure as if it were part of

the outer function. And so directly analyzes 'x = 1' or

'opaque(x)' and sees that neither exposes the address of x

and therefore finds no evidence from those uses that

x needs to move to the heap.

On Tue, Feb 12, 2019 at 3:38 AM Matthew Dempsky <mdem...@google.com> wrote:

So working from this example again:

    //go:noinline

    func g(f func() error) error {

        return f()

    }

    func H() error {

        var y int

        return g(func() error {

            y++

            return nil

        })

    }

There are a few relevant details about how esc.go handles this code:

1. It first analyzes g and summarizes it as "~r0 = *f" (using EscReturnBits in the esc tags). This is fine.

2. At L1064, we associate "H.func1 = &y" because y is captured by reference.

3. At L1269, there's code in escassign to rewrite "dst = H.func1" into "dst = &H.func1", since function literals are actually a reference type.

4. The call to "g(H.func1)" gets handled by escassignfromtag, which rewrites to ".out0 = *H.func1" (based on the summary from step 1 above) and calls escassign. (See L1479.)

The problem is the interaction between 3 and 4: escassign doesn't recurse on src when src.Op == ODEREF, so ".out0 = *H.func1" is left as-is, rather than being rewritten to ".out0 = *&H.func1".

#2 and #3 are describing the code that takes care of making sure

that when H.func1 escapes, &y is understood to escape as well.

They are not relevant to what happens when H.func1 does not

escape and is merely called.

In contrast, #1 and #4 are about tracking the flow of g's arguments

to its results. In this case that doesn't make much sense.

You can see that the return-tracking bits don't really matter for

deciding what happens to &y by trying this example in the

current escape analysis:

func call(f func() *int) *int { f(); return nil }

func h() { var y int; call(func() *int { return &y }) }

Or even this:

func h() { var y int; _ = func() *int { return &y } }

Both of them decide that &y escapes, and there isn't

even a call to analyze in the second.

Again, I am thrilled you are cleaning up this code and also

that there are so many new cases that can be kept on the stack.

But let's make sure to keep these on the stack too.

Thanks very much.

Best,

Russ

[Matthew Dempsky]

On Wed, Feb 13, 2019 at 9:14 AM Russ Cox <r...@golang.org> wrote:

But both of these cases seem worth preserving

if at all possible, and it does look possible to me.

Having now understood the issue, I think this should be easy to fix. We already treat values returned by functions (including closures) as escaping to the heap, so I think there's really no point in worrying that f() might return a value from within the closure. We only need to worry about leaking f itself.

I'll investigate addressing this.

I remain convinced though that the regressions are related to an off-by-1 error in the current code though that happens to get lucky in these cases, and not by design.

As a simple example, H1 and H2 here should be identical:

    func call(f func() error) error { return f() }

    func H1() error {

        var x int

        return call(func() error { x++; return nil })

    }

    func H2() error {

        var x int

        tmp := func() error { x++; return nil }

        return call(tmp)

    }

If you compile this with -m=3, you'll see these two distinct path traces from ~r0 (H1/H2's unnamed return parameter):

escflood:2: dst ~r0 scope:H1[0]

escwalk: level:{0 0} depth:0  op=NAME .out0( l(7) x(0) class(PAUTO) ld(1) used) scope:H1[1] extraloopdepth=-1

escwalk: level:{0 0} depth:1 op=* *(func literal)( l(7) ld(1)) scope:H1[1] extraloopdepth=-1

escwalk: level:{1 1} depth:2 op=CLOSURE func literal( l(7) esc(no) ld(1) tc(1)) scope:H1[1] extraloopdepth=-1

escwalk: level:{1 1} depth:3 op=& &x( l(7) ld(1) tc(1)) scope:H1[1] extraloopdepth=-1

escwalk: level:{0 0} depth:4 op=NAME x( a(true) g(2) l(6) x(0) class(PAUTO) ld(1) tc(1) addrtaken assigned used) scope:H1[1] extraloopdepth=-1

escflood:3: dst ~r0 scope:H2[0]

escwalk: level:{0 0} depth:0  op=NAME .out0( l(13) x(0) class(PAUTO) ld(1) used) scope:H2[1] extraloopdepth=-1

escwalk: level:{0 0} depth:1 op=* *tmp( l(12) ld(1)) scope:H2[1] extraloopdepth=-1

escwalk: level:{1 1} depth:2 op=NAME tmp( a(true) g(2) l(12) x(0) class(PAUTO) ld(1) tc(1) used) scope:H2[1] extraloopdepth=-1

escwalk: level:{1 1} depth:3 op=& &(func literal)( l(12) ld(1)) scope:H2[1] extraloopdepth=-1

escwalk: level:{0 0} depth:4 op=CLOSURE func literal( l(12) esc(no) ld(1) tc(1)) scope:H2[1] extraloopdepth=-1

z.go:12:9: func literal escapes to heap

    [ "from" path elided ]

escwalk: level:{0 0} depth:5 op=& &x( l(12) ld(1) tc(1)) scope:H2[1] extraloopdepth=1

z.go:12:24: &x escapes to heap, level={0 0}, dst=~r0 dst.eld=0, src.eld=1

z.go:11:6: moved to heap: x

escwalk: level:{-1 -1} depth:6 op=NAME x( a(true) g(2) l(11) x(0) class(PAUTOHEAP) esc(h) ld(1) tc(1) addrtaken assigned used) scope:H2[1] extraloopdepth=1

Ignore the "func literal escapes to heap" bit for a moment. That's a red herring, which I'll explain below.

Note that if we elide the tmp ONAME (which shouldn't affect escape analysis), we get these two traces:

    H1: ~r0 .out0 * CLOSURE & x

    H2: ~r0 .out0 * & CLOSURE & x

I assert that the graph for H1 is erroneous here: it's missing that extra & operation.

The reason is as I outlined before. escassign is supposed to rewrite "dst = OCLOSURE" into "dst = & OCLOSURE". This works okay for H2 because it's able to rewrite "tmp = OCLOSURE" into "tmp = & OCLOSURE".

However, in H2, escassignfromtag has already setup ".out0 = * OCLOSURE", and escassign fails to insert the "&" operation, because src.Op is OADDR rather than OCLOSURE.

Now, let me address the "func literal escapes to heap" message. It's easy to mistake that this is why x is moved to the heap: if the closure legitimately leaked, of course any variables it closes over need to be leaked too. However, if you comment out "case OCLOSURE" in escwalkBody (so that closure's never escape--an unsafe change), x still escapes to heap in H2.

As for why H2's function literal escapes, this happens because there's another off-by-1 error in closure handling, because esc.go is inconsistent about whether OCLOSURE represents the closure-pointer or the closure object itself. Equivalently, is it like OPTRLIT or OSTRUCTLIT?

escassign tries to insert the "&" operation, because it thinks OCLOSURE is like OSTRUCTLIT; but escwalkBody treats it like OPTRLIT (see that it's handled the same as other pointer-returning operations like ONEW and OMAKESLICE).

For what it's worth, my rewrite mitigates these sorts of issues because:

  1. It uses a data-flow graph that's completely separate from the AST, whereas esc.go still relies on the AST during graph walking.

  2. It constructs the data-flow graph in a single unified one-pass AST traversal, whereas esc.go uses multiple overlapping recursive AST traversals that try to assemble different pieces of the graph.

[Russ Cox]

On Wed, Feb 13, 2019 at 7:24 PM Matthew Dempsky <mdem...@google.com> wrote:

I remain convinced though that the regressions are related to an off-by-1 error in the current code though that happens to get lucky in these cases, and not by design.

It's easy to believe there are bugs

cancelling each other out or going unnoticed.

But the code does not just "happen to get lucky".

Repeating that so many times dismisses the significant

engineering work by others and is frankly a bit off-putting.

They wrote lots of tests and made sure the code

passed those tests. The tests are the reason

the code works on these cases, not luck.

Thanks again for the rewrite and for figuring out

how to handle this case more cleanly.

Best,

Russ

[lab...@gmail.com]

I am finishing up some work that was started by Michael Strosaker to port the s390x asm implementation of crypto/elliptic to ppc64le. I hope I can get it working in time for Go 1.13 with some help from the golang s390x team.

I have several others areas I'd like to work on but require some investigation and understanding of what needs to be done. If the improvements to the ABI are ready to be implemented, we'd like to leverage those as much as possible. I'm not sure how that work is to be divided up and exactly needs to be done. That should provide a nice improvement for ppc64x.

Loops are also an area that could be improved for ppc64x, but that also requires some investigation to compare loops generated by gccgo and golang and see how much benefit a new sequence might provide. That would initially be using the ctr more often instead of compares and branches. We also need more hoisting of constants and other invariants that doesn't seem to be happening, but as with the other, not sure on how much the benefit will be, but we should be able to measure that with testcases built with gccgo vs. golang containing the types of loops that could be improved.

Other than those there are a few minor rules changes that could be done. We could do an improved bswap but unfortunately on ppc64x that requires a load and store to memory to do a byte-reversed load or store.

[Matthew Dempsky]

On Wed, Feb 13, 2019 at 5:23 PM Russ Cox <r...@golang.org> wrote:

It's easy to believe there are bugs

cancelling each other out or going unnoticed.

But the code does not just "happen to get lucky".

Repeating that so many times dismisses the significant

engineering work by others and is frankly a bit off-putting.

I’m really sorry. I don't mean to dismiss the engineering effort put into esc.go. I know you, David, Cherry, and others have put very significant work into improving escape analysis, investigating issues, writing test cases, and more over the years. I've always been impressed with that work. esc.go is a very subtle body of code that has accumulated a lot of complexity as a result of its long history. I'm personally thankful for the extensive tests written for it: they proved very instructive while I was learning how esc.go works.

By "not by design," I meant just that the interaction of esc.go routines that resulted in those objects being stack allocated seems unintentional; and by "happens to get lucky," I was referring to the affected code patterns were indeed always safe to stack allocate. I suggested this as a hypothesis earlier, and David supported that assessment. Moreover, having spent a while understanding esc.go, I agreed with his nervousness about the closure handling code in particular. In that context, I thought David agreed with my phrasing when he repeated it, and I used it again in two more emails (which were otherwise focused on factually discussing the code) to make clear I was continuing to refer back to that same original hypothesis.

I certainly did not mean to suggest esc.go was not designed or not thoughtfully engineered. I did not mean any personal offense to anyone.

That said, I found your earlier response very frustrating.

I have a lot of respect for you and your work, so when you showed a modicum of enthusiasm for mine and asked about regressions, I dropped everything to get you an up-to-date comparison. I also detailed those minority of cases that regressed, and then spent hours digging into the root cause. I’d already spent days looking into them before, but this time I was motivated by wanting to impress you. And I did track down why and explained it. I was brief to respect your time and general expertise, but I would have been happy to elaborate.

But your response? First, you lectured me about closure capture-by-value by capture-by-reference, as though that’s not something any experienced C++ programmer knows or covered in introductory compiler texts. As though I was specifying “captured by reference” in all of my emails because I just like to be verbose.

Then, you didn’t believe me when I’d already enumerated all of the regressions for you and said they were the same issue. (Sorry, I over simplified: the par.Cache.Do regressions actually involve esc.go:1465 rather than esc.go:1482.) No inquiry into why I thought that. Instead, you immediately started listing additional issues you assumed were in my code and asserted those were the actual faulty code patterns. Well sorry, you lost your “bet”: my rewrite handles both of your “g” functions without escaping x to the heap.

But most annoyingly, you dismissed my explanation and then misexplained how the existing code works. “The return-tracking bits don't really matter”? Then explain why new(int) only escapes in F1 here. The only difference is whether the call* function returns the result of f().

And do you know why new(int) escapes there, but y doesn’t? Because there’s an off-by-1 calculation in esc.go’s connectivity-graph edge construction code. Let me disregard your decades of programming expertise to patronizingly remind you: off-by-1 errors have a long history of causing issues and security vulnerabilities. And in the context of esc.go specifically, both of the memory corruption errors I linked above were instances of this same problem. So the fact that here another off-by-1 error instead resulted in a safe compiler optimization? I’d love to know how you describe that other than “lucky.”

I put weeks of effort into thoroughly understanding esc.go and wrote a new implementation that halves the code complexity, reduces compiler memory usage, fixes multiple memory corruption issues (#29197, #29353), and overall significantly improves escape analysis results. I think I deserve some credit that maybe I understand how esc.go works now.

Yes, my rewrite currently causes y to unnecessarily escape too, which is a legitimate performance regression concern. And yes, because of this tedious exchange I’m going to improve it even further so that neither y nor new(int) escape.

But your email was factually wrong, and I think it’s fair for me to have stood up for myself about that. And even though your email was also obnoxious, instead of publicly calling you out on that or directly saying “you’re wrong,” I charitably assumed the misunderstanding was because I inadequately explained the problem in the first place, so I tried again to give a clearer example.

Now, instead of owning up to your error, you instead suddenly take offense with my phrasing.

esc.go was described earlier in this very thread by another senior Go engineer as “has never been very maintainable” and “so complex” and mentioned its “lack of documentation,” and you even “seconded” their message. But calling esc.go “lucky” is the affront to engineering? Please.

They wrote lots of tests and made sure the code

passed those tests. The tests are the reason

the code works on these cases, not luck.

There are no escape analysis tests that cover the regression cases under discussion. Please don't attack me for pointing this out too.

[David Chase]

I agree with Matthew's analysis here.  The existing escape analysis code is not confidence-inspiring, and though it has many tests, it does not have all the tests it needs (and it's my fault for not adding more; I can think of at least one build-breaking change that I tried, that nonetheless passed all the tests by itself).  "Lucky" is also an appropriate word to describe closure handling in the existing escape analysis.

I would really like to see Matthew's rewrite of escape analysis get into 1.13.  If the few escape regressions turn out to have measurable performance regressions, then we need to try to fix that, but I'd also consider eating that cost or trying to make it up elsewhere, to get the better and more trustworthy code into the compiler.

--

[Ralph Corderoy]

Hi Matthew,

> And even though your email was also obnoxious, instead of publicly
> calling you out on that or directly saying “you’re wrong,”

> I charitably <https://golang.org/conduct> assumed the misunderstanding

> was because I inadequately explained the problem in the first place,
> so I tried again to give a clearer example.

I think you can do that but still stay within those rules of conduct by
stating that you think you've been misunderstood, perhaps you weren't
clear, they haven't grasped you point, etc. Then the annoyance isn't
being bottled up and the other party gets a prompt to consider what
they've missed. :-)

> So the fact that here another off-by-1 error instead resulted in a
> safe compiler optimization? I’d love to know how you describe that
> other than “lucky.”

IME it's quite common to have two off-by-one errors `cancel' each other
in complex areas, especially with test-driven development as the passing
of tests helps reassure against the nagging doubt the logic isn't fully
understood. So is it more goal-directed hill climbing than luck?

--
Cheers, Ralph.

[Russ Cox]

Hi Matthew,

I'm really sorry to have caused you such frustration. I apologize. As I said, I really do appreciate all the work you are putting into this, and I am thrilled that the code will be cleaned up and no longer be something people are afraid to try to work on.

The point of my long mail was only to try to build an intuition for why it would be reasonable to expect escape analysis to still handle the regressions you pointed out, based on the original whiteboard design. I apologize that I did not get some details right, and I apologize that it seemed like I was lecturing or assuming you didn't know certain things. As you're well aware, the Go implementation is hardly "textbook", escape analysis least of all, so I try to avoid assuming textbook concepts.

I still think the regexp srepl case is important (I believe that variable has been stack-allocated since before Go 1), and it sounds like you've got a fix for it, which is great. Perhaps a better, shorter mail would have been to only point out the regexp srepl case as something that seems really important to leave on the stack, and not try to analyze why or reduce the case without access to a specific implementation.

I also apologize for the criticism about calling code lucky. I was trying to push back on the general tendency I've observed from many people, especially with respect to the more subtle parts of the Go compiler and runtime, to assume that code that doesn't make sense must be working entirely by accident. But I was wrong to suggest that you were falling into that trap. (It was a big aha moment for me when I realized that the answer to "how could this possibly have worked?" is almost always "because there was a test".)

More important than any detail of escape analysis, I want you to be happy contributing to Go, and I want you to know how much I personally appreciate your contributions. That's the most important thing. Thank you.

Best,

Russ

[Matthew Dempsky]

Hi Russ,

Thank you for the sincere and heart felt apology. It means a lot to me.

I'm sorry for resorting to sarcasm in my last email to express myself.

On Fri, Feb 15, 2019 at 9:35 AM Russ Cox <r...@golang.org> wrote:

I also apologize for the criticism about calling code lucky. I was trying to push back on the general tendency I've observed from many people, especially with respect to the more subtle parts of the Go compiler and runtime, to assume that code that doesn't make sense must be working entirely by accident.

I appreciate this desire.

For my part, I make an effort to frame my comments on code as either (1) objective factual statements or (2) clearly personal subjective statements (and either way framed constructively). For example, in my initial report at #23109, I tried to highlight particular difficulties I had understanding the escape analysis documentation as my failing to understand it (as opposed to the code's failing to be accessible to me) and highlighting them as opportunities for improvement.

I filed that issue over a year ago. Re-reading it today, I see plenty of ways I could have better expressed myself. I hope though the effort still shows.

(And in the interest of clarity, I do not perceive your description of that general tendency to be directed at me, and I'm not trying to defend myself here. I'm just sharing a similar small act I try to take to similarly encourage a more positive engineering culture.)

(It was a big aha moment for me when I realized that the answer to "how could this possibly have worked?" is almost always "because there was a test".)

Yes, I definitely appreciate Go's testing culture. Working on the compiler would be nowhere as productive (and in turn enjoyable) as it is today without the extensive regression test suite and tools like toolstash -cmp (thank you again for writing that).

Cheers,

Matthew

[Matthew Dempsky]

Hi Ralph,

On Fri, Feb 15, 2019 at 6:10 AM Ralph Corderoy <ra...@inputplus.co.uk> wrote:

I think you can do that but still stay within those rules of conduct by
stating that you think you've been misunderstood, perhaps you weren't
clear, they haven't grasped you point, etc.  Then the annoyance isn't
being bottled up and the other party gets a prompt to consider what
they've missed.  :-)

Thanks. In retrospect, I think that would have helped. I'll try to do that in the future.

IME it's quite common to have two off-by-one errors `cancel' each other
in complex areas, especially with test-driven development as the passing
of tests helps reassure against the nagging doubt the logic isn't fully
understood.  So is it more goal-directed hill climbing than luck?

I think there are parts of esc.go that fit your description, and I agree "lucky" is an inappropriate way to describe code passing in cases that are covered by tests. But there were no tests for the particular cases under discussion.

David agrees "lucky" was appropriate here, and Russ has withdrawn his concern over its use here. I think we can consider this issue resolved.

Thanks,

Matthew