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

Comments

@randall77
Copy link
Contributor

commented Dec 18, 2013

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

This comment has been minimized.

Copy link
Contributor

commented Apr 3, 2014

Comment 1:

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

@rsc

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

Comment 7:

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 15, 2014

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Member

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

This comment has been minimized.

Copy link
Contributor

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

This comment has been minimized.

Copy link
Contributor

commented Oct 1, 2014

Comment 14:

Labels changed: added repo-main.

@rsc rsc added this to the Unplanned milestone Apr 10, 2015

@rsc rsc removed release-none labels 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

@dbentley dbentley referenced this issue Jun 16, 2016

@bradfitz bradfitz modified the milestones: Go1.8, Unplanned Jul 17, 2016

@quentinmit quentinmit added the NeedsFix label Oct 11, 2016

@randall77

This comment has been minimized.

Copy link
Contributor Author

commented Dec 12, 2018

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

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

@gopherbot

This comment has been minimized.

Copy link

commented Apr 23, 2019

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

@gopherbot

This comment has been minimized.

Copy link

commented Jun 7, 2019

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
cmd/link: fix deferreturn detector
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>
@changkun changkun referenced this issue Jun 19, 2019
0 of 16 tasks complete
@WGH-

This comment has been minimized.

Copy link

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

This comment has been minimized.

Copy link
Contributor Author

commented Sep 11, 2019

Yes, this is done and should be closed.

@randall77 randall77 closed this Sep 11, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
You can’t perform that action at this time.