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.

Sign up for GitHub

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: skip ptr writeback for appends which don't grow #14969

Closed
randall77 opened this issue Mar 25, 2016 · 5 comments
Closed

cmd/compile: skip ptr writeback for appends which don't grow #14969

randall77 opened this issue Mar 25, 2016 · 5 comments
Labels
FrozenDueToAge Performance
Milestone
Unplanned

Comments

@randall77
Copy link
Contributor 

func(t *[]int) {
    *t = append(*t, 7)
}

We only need to write the pointer (and the capacity) back when we take the growslice path. For the standard path, only the length needs to be written back.

See #14921 and #14855 .
@randall77 randall77 mentioned this issue Mar 29, 2016
Closed
@randall77
Copy link
Contributor Author

Looks like this is related to the following in SSA (ssa.go:OAS case):

        if rhs != nil && rhs.Op == OAPPEND {
            // Yuck!  The frontend gets rid of the write barrier, but we need it!
            // At least, we need it in the case where growslice is called.
            // TODO: Do the write barrier on just the growslice branch.
            // TODO: just add a ptr graying to the end of growslice?
            // TODO: check whether we need to do this for ODOTTYPE and ORECV also.
            // They get similar wb-removal treatment in walk.go:OAS.
            needwb = true
        }

So we need to be less aggressive in adding back the write barrier. The frontend basically punts to the backend to decide if write barriers are needed (walk.go, OAS case):

            if r.Op == OAPPEND {
                // Left in place for back end.
                // Do not add a new write barrier.
                break opswitch
            }

For *p = append(*p, x) we need to generate code like:

a := *p
newlen := a.len+1
if newlen > a.cap {
   a = growslice()...
   p.ptr = a.ptr  // with a write barrier
   p.cap = a.cap
}
p.len = newlen
a.ptr[newlen-1] = x // with a write barrier, if needed

This code is similar to what we generate for the generic OAPPEND case, maybe we can share some code.

See #15023 for another case of this same problem.

@randall77 randall77 mentioned this issue Mar 29, 2016
Closed
@josharian
Copy link
Contributor

I'll take a look at this.

@bradfitz bradfitz added this to the Unplanned milestone Apr 9, 2016
@gopherbot
Copy link
Contributor

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

@gopherbot
Copy link
Contributor

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

@josharian josharian reopened this Apr 12, 2016
gopherbot pushed a commit that referenced this issue Apr 12, 2016
@josharian
cmd/compile: temporarily disable inplace append special case
811ebb6 
Fixes #15246
Re-opens #14969

Change-Id: Ic0b41c5aa42bbb229a0d62b7f3e5888c6b29293d
Reviewed-on: https://go-review.googlesource.com/21891
Run-TryBot: Josh Bleecher Snyder <josharian@gmail.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Keith Randall <khr@golang.org>
@gopherbot
Copy link
Contributor

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

@golang golang locked and limited conversation to collaborators Apr 19, 2017
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Assignees
No one assigned
Labels
FrozenDueToAge Performance
Projects
None yet
Milestone
Unplanned
Development

No branches or pull requests
4 participants
@bradfitz @josharian @randall77 @gopherbot