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

cmd/compile: allocate some defers in stack frames #6980

Closed
randall77 opened this issue Dec 18, 2013 · 21 comments
Closed

cmd/compile: allocate some defers in stack frames #6980

randall77 opened this issue Dec 18, 2013 · 21 comments
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Milestone

Comments

@randall77
Copy link
Contributor

When we can statically determine that a defer is called (at most) once, we should
allocate the Defer structure in the stack frame instead of from the DeferChunk mechanism.

We suspect most defers fit into this category.
@rsc
Copy link
Contributor

rsc commented Apr 3, 2014

Comment 1:

Labels changed: added release-go1.4, removed release-go1.3maybe.

@rsc
Copy link
Contributor

rsc commented May 9, 2014

Comment 2:

I sketched this during gophercon. The 6g-only CL is in
https://golang.org/cl/100300043 although it also has an unrelated ... fix for
escape analysis mixed in.
It made basically no difference for things that hit Dmitriy's latest defer cache. For
strangely-sized defers it might help and be worth doing.

@dgryski
Copy link
Contributor

dgryski commented Jun 10, 2014

Comment 3:

Some simple benchmarks from expvar-like locked values:
BenchmarkDeferAddInt    20000000            91.4 ns/op
BenchmarkAddInt 100000000           21.6 ns/op
BenchmarkDeferAddFloat  20000000            89.3 ns/op
BenchmarkAddFloat   100000000           21.7 ns/op
So, the overhead is pretty big.

@dgryski
Copy link
Contributor

dgryski commented Jun 10, 2014

Comment 4:

Some simple benchmarks from expvar-like locked values:
BenchmarkDeferAddInt    20000000            91.4 ns/op
BenchmarkAddInt 100000000           21.6 ns/op
BenchmarkDeferAddFloat  20000000            89.3 ns/op
BenchmarkAddFloat   100000000           21.7 ns/op
So, the overhead is pretty big.

@rsc
Copy link
Contributor

rsc commented Sep 15, 2014

Comment 5:

Stack copying now depends on Defers NEVER being on the stack.

Labels changed: added release-none, removed release-go1.4.

Status changed to Retracted.

@dvyukov
Copy link
Member

dvyukov commented Sep 15, 2014

Comment 6:

The code in stack.c suggests the opposite:
    for(dp = &gp->defer, d = *dp; d != nil; dp = &d->link, d = *dp) {
        if(adjinfo->old.lo <= (uintptr)d && (uintptr)d < adjinfo->old.hi) {
            // The Defer record is on the stack.  Its fields will
            // get adjusted appropriately.
            // This only happens for runtime.main and runtime.gopanic now,
            // but a compiler optimization could do more of this.
            // If such an optimization were introduced, Defer.argp should
            // change to have pointer type so that it will be updated by
            // the stack copying. Today both of those on-stack defers
            // set argp = NoArgs, so no adjustment is necessary.
            *dp = (Defer*)((byte*)d + adjinfo->delta);
            continue;
        }

Labels changed: added release-go1.5, removed release-none.

Status changed to Accepted.

@randall77
Copy link
Contributor Author

Comment 7:

Russ is a bit ahead of himself - this will be the case when
https://golang.org/cl/141490043/ is in.

@dvyukov
Copy link
Member

dvyukov commented Sep 15, 2014

Comment 8:

Why defers on stack won't work? Are there any principal problems? Don't we just need to
do proper adjustments to support defers on stack?

@dvyukov
Copy link
Member

dvyukov commented Sep 15, 2014

Comment 9:

Defers are still super slow. To the point where one can prefer to not use defer at all
just because of the performance.

@randall77
Copy link
Contributor Author

Comment 10:

I don't think there is anything in principle that wouldn't work.  You just have to be
careful about avoiding tracebacks while copying the stack.

@rsc
Copy link
Contributor

rsc commented Sep 15, 2014

Comment 11:

It is probably possible to make work. However, I benchmarked it (patch in the CL above)
and it just doesn't matter. The defer cache takes care of nearly all the win. It is not
worth it.
I will leave this open but it is not targeted to a release.

Labels changed: added release-none, removed release-go1.5.

@dvyukov
Copy link
Member

dvyukov commented Sep 15, 2014

Comment 12:

The compiler also needs to *fill* in the struct and link it into g->defer, and on
successful return unlink from g->defer inline. That would provide the speedup.
Today the single defer in net.Conn.Read/Write consumes 2.5% of time in the end-to-end
HTTP benchmark:
http://build.golang.org/log/33d55778f33682e4e1d45e7c825a73d2ea500de7
Defer is not something that must be visible in a profile of such program. We don't want
to wipe defers from std lib due to performance, right? I guess there are Go users who
already do this with their libraries.

@rsc
Copy link
Contributor

rsc commented Sep 16, 2014

Comment 13:

Okay, well that might work. Defers will need more care in the stack copier if they get
stack-allocated, because the Defer chain will weave between allocated memory and the
stack, but I think it is possible to make it work. (In contrast it was fundamentally not
possible to make stack-allocated Panics work when they were also used during traceback,
because a Panic near the bottom of the stack wasn't actually needed until later up the
stack, and by then it had been rewritten. The CL 141490043 fixed this by not using
Panics during traceback anymore.)

@griesemer
Copy link
Contributor

Comment 14:

Labels changed: added repo-main.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015
@rsc rsc changed the title cmd/gc: allocate some defers in stack frames cmd/compile: allocate some defers in stack frames Jun 8, 2015
@bradfitz bradfitz modified the milestones: Go1.8, Unplanned Jul 17, 2016
@quentinmit quentinmit added the NeedsFix The path to resolution is known, but the work has not been done. label Oct 11, 2016
@bradfitz bradfitz added early-in-cycle A change that should be done early in the 3 month dev cycle. and removed early-in-cycle A change that should be done early in the 3 month dev cycle. labels Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10Early, Go1.10 Jun 14, 2017
@bradfitz bradfitz modified the milestones: Go1.10, Go1.11 Nov 28, 2017
@bradfitz bradfitz modified the milestones: Go1.11, Go1.12 Jun 19, 2018
@randall77
Copy link
Contributor Author

Punting to unplanned, too late for anything major in 1.12.

@randall77 randall77 modified the milestones: Go1.12, Unplanned Dec 12, 2018
@gopherbot
Copy link

Change https://golang.org/cl/171758 mentions this issue: cmd/compile,runtime: allocate defer records on the stack

@gopherbot
Copy link

Change https://golang.org/cl/181258 mentions this issue: cmd/link: fix deferreturn detector

gopherbot pushed a commit that referenced this issue Jun 7, 2019
The logic for detecting deferreturn calls is wrong.

We used to look for a relocation whose symbol is runtime.deferreturn
and has an offset of 0. But on some architectures, the relocation
offset is not zero. These include arm (the offset is 0xebfffffe) and
s390x (the offset is 6).

This ends up setting the deferreturn offset at 0, so we end up using
the entry point live map instead of the deferreturn live map in a
frame which defers and then segfaults.

Instead, use the IsDirectCall helper to find calls.

Fixes #32477
Update #6980

Change-Id: Iecb530a7cf6eabd7233be7d0731ffa78873f3a54
Reviewed-on: https://go-review.googlesource.com/c/go/+/181258
Run-TryBot: Keith Randall <khr@golang.org>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Cherry Zhang <cherryyz@google.com>
@WGH-
Copy link

WGH- commented Sep 6, 2019

Should this be closed? From what I understand, this has been temporarily reverted due to some bugs, and then reverted back: fff4f59 -> 49200e3 -> 8f296f5

@randall77
Copy link
Contributor Author

Yes, this is done and should be closed.

@golang golang locked and limited conversation to collaborators Sep 10, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
early-in-cycle A change that should be done early in the 3 month dev cycle. FrozenDueToAge NeedsFix The path to resolution is known, but the work has not been done. Performance
Projects
None yet
Development

No branches or pull requests