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

runtime: remove implementation restriction on channel element size #9120

Open
bradfitz opened this issue Nov 17, 2014 · 18 comments
Open

runtime: remove implementation restriction on channel element size #9120

bradfitz opened this issue Nov 17, 2014 · 18 comments
Milestone

Comments

@bradfitz
Copy link
Contributor

@bradfitz bradfitz commented Nov 17, 2014

It appears that gc and the runtime currently disable channel elements to be over 64kB:

    http://play.golang.org/p/_aVWt-qiud

But I see nothing about this (allowed?) implementation restriction in the spec, despite
the spec calling out a number of other implementation-allowed restrictions.
@minux
Copy link
Member

@minux minux commented Nov 17, 2014

Comment 1:

ideally the spec should also set a minimum requirement on the item size limit.

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 18, 2014

Comment 2:

What's the implementation issue requiring a limit in the first place (besides memory
availability)? (Array don't have a limit, for instance).

@rsc
Copy link
Contributor

@rsc rsc commented Nov 18, 2014

Comment 3:

Ken put the limit there. I have always assumed it was something like the "stupid shift"
errors, where if you have that big an element size you are probably doing something
wrong. The fact that it took this long for someone to report a problem suggests he was
right. (And even Brad doesn't seem like he's actually run into this.)
I can take it out for 1.5.

@bradfitz
Copy link
Contributor Author

@bradfitz bradfitz commented Nov 18, 2014

Comment 4:

Correct. I was just checking something in the runtime and noticed in chan.go:
func makechan(t *chantype, size int64) *hchan {
        elem := t.elem
        // compiler checks this but be safe.
        if elem.size >= 1<<16 {
                gothrow("makechan: invalid channel element type")
        }
 
And it just surprised me to see it in either place, much less both places.

@robpike
Copy link
Contributor

@robpike robpike commented Nov 18, 2014

Comment 5:

Let's just remove the restriction from the compiler and replace it with a
overall buffer size check.
-rob

@griesemer
Copy link
Contributor

@griesemer griesemer commented Nov 18, 2014

Comment 6:

Labels changed: removed documentation.

Owner changed to ---.

@bradfitz bradfitz added this to the Go1.5 milestone Dec 16, 2014
@bradfitz bradfitz added this to the Go1.5 milestone Dec 16, 2014
@rsc rsc removed accepted labels Apr 14, 2015
@josharian
Copy link
Contributor

@josharian josharian commented May 5, 2015

Let's just remove the restriction from the compiler and replace it with a overall buffer size check.

The buffer size might not be constant, so we can't check it during compilation. Do we still want to pull the element size check?

@randall77
Copy link
Contributor

@randall77 randall77 commented May 5, 2015

Easy enough to remove the hchan.elemsize field and use hchan._type.size instead. One extra indirection, but not a big deal.

@josharian
Copy link
Contributor

@josharian josharian commented May 5, 2015

It is dataqsiz that we don't know at compile time. The argument against changing this is that, right now, if your code compiles, then it won't panic during makechan. That seems like a property worth keeping.

@randall77
Copy link
Contributor

@randall77 randall77 commented May 5, 2015

I don't understand. Currently you can get a panic during makechan. If you allocate 2^30 objects each of size 2^8, for example. Removing the elemsize restriction won't make that any worse (or better).

@minux
Copy link
Member

@minux minux commented May 5, 2015

@josharian
Copy link
Contributor

@josharian josharian commented May 6, 2015

Ah. Indeed, thanks.

@rsc
Copy link
Contributor

@rsc rsc commented Jun 8, 2015

The 64 kB limit turns out to be central to a fix for a late GC bug in the Go 1.5 release. I'm going to take advantage of this 64 kB limit to fix that bug. We can try to remove the limit (with a different fix) in 1.6.

@rsc rsc changed the title cmd/gc: remove implementation restriction on channel element size runtime: remove implementation restriction on channel element size Jun 8, 2015
@rsc rsc added this to the Unplanned milestone Jun 8, 2015
@rsc rsc removed this from the Go1.5 milestone Jun 8, 2015
@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 6, 2022

Are we decided here that implementations should support channels with element sizes >=64KiB? I think we should, but want to double check that before implementing it.

It seems like the easy way to handle it is that overly-large elements just get passed around indirectly, like how we handle map keys/elements >128 bytes.

FWIW, it looks like gccgo allows compiling code with channels with element sizes >=64KiB, but makeing them fails at runtime.

This is relevant to generics, because it's an end user error we can't report until after instantiation. Getting rid of the error altogether would simplify compiler internals, and also better fulfill the generic design principle that any type arguments that satisfy the type parameter constraints is a valid instantiation.

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 7, 2022

So the runtime has this comment:

// Sends and receives on unbuffered or empty-buffered channels are the
// only operations where one running goroutine writes to the stack of
// another running goroutine. The GC assumes that stack writes only
// happen when the goroutine is running and are only done by that
// goroutine. Using a write barrier is sufficient to make up for
// violating that assumption, but the write barrier has to work.
// typedmemmove will call bulkBarrierPreWrite, but the target bytes
// are not in the heap, so that will not help. We arrange to call
// memmove and typeBitsBulkBarrier instead.

Can we just have escape analysis move all variables >=64KiB that are involved in channel communication to the heap? Then sendDirect/recvDirect can just use typedmemmove for GC prog types, since it knows those will be heap allocated? (typeBitsBulkBarrier doesn't support GC programs, and it seems like GC programs aren't easy to interpret in place without extra buffer space for interpreting "repeat" instructions.)

@randall77
Copy link
Contributor

@randall77 randall77 commented Jun 7, 2022

The runtime also has this comment:

if r.useGCProg() {
			// This path is pretty unlikely, an object large enough
			// to have a GC program allocated on the stack.
			// We need some space to unpack the program into a straight
			// bitmask, which we allocate/free here.
			// TODO: it would be nice if there were a way to run a GC
			// program without having to store all its bits. We'd have
			// to change from a Lempel-Ziv style program to something else.
			// Or we can forbid putting objects on stacks if they require
			// a gc program (see issue 27447).
			s = materializeGCProg(r.ptrdata(), gcdata)
			gcdata = (*byte)(unsafe.Pointer(s.startAddr))
		}

It's the same really weird wart, and would be solved by not allowing objects with GC progs on the stack. So maybe we should never allocate GCprog'd objects on the stack? Or maybe even all >64KB objects?

@mdempsky
Copy link
Member

@mdempsky mdempsky commented Jun 7, 2022

Moving all GC-program types to the heap looks like it would be easily doable.

@gopherbot
Copy link

@gopherbot gopherbot commented Jun 7, 2022

Change https://go.dev/cl/410895 mentions this issue: [dev.unified] runtime: allow channel element types >=64KiB

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

9 participants