|
|
Descriptionruntime: convert channel operations to Go, part 1 (chansend1).
Patch Set 1 #Patch Set 2 : diff -r 3cf190969915 https://code.google.com/p/go/ #Patch Set 3 : diff -r 3cf190969915d6d531acd0795eb81974aaa64d19 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 4 : diff -r 3cf190969915d6d531acd0795eb81974aaa64d19 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 5 : diff -r 3cf190969915d6d531acd0795eb81974aaa64d19 https://khr%40golang.org@code.google.com/p/go/ #
Total comments: 11
Patch Set 6 : diff -r 3cf190969915d6d531acd0795eb81974aaa64d19 https://khr%40golang.org@code.google.com/p/go/ #Patch Set 7 : diff -r a457912af7d057d5ae74008b2166c02f362311ad https://khr%40golang.org@code.google.com/p/go/ #Patch Set 8 : diff -r a457912af7d057d5ae74008b2166c02f362311ad https://khr%40golang.org@code.google.com/p/go/ #Patch Set 9 : diff -r 79cbd2059d28a32bd61779e27911746469f3776c https://khr%40golang.org@code.google.com/p/go/ #Patch Set 10 : diff -r 0bb034265fafa98788400b2f64cb17154fef11eb https://khr%40golang.org@code.google.com/p/go/ #
Total comments: 9
Patch Set 11 : diff -r 0bb034265fafa98788400b2f64cb17154fef11eb https://khr%40golang.org@code.google.com/p/go/ #
Total comments: 1
MessagesTotal messages: 20
Hello dvyukov@google.com (cc: golang-codereviews@googlegroups.com), I'd like you to review this change to https://khr%40golang.org@code.google.com/p/go/
Sign in to reply to this message.
first round https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go File src/pkg/runtime/chan.go (right): https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M stack. Is it bad? This function does not need to be in Go. https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) now that makechan is in Go can we just say: c = new(hchan) instead of: c = (*hchan)(newobject(&hchanType)) then we don't need chaninit at all https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { please move this to the bottom of the file this is not the most important function in the file and this is Go now https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, lock *lock, reason string) { move this to proc.go https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason string) { move this to proc.go https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:133: func goready(gp *g) { move this to proc.go https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, elem unsafe.Pointer) { this must be NOSPLIT as you call gogetcallerpc https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:194: mysg := &gp.chansudog How do you want to handle select sudog's? When we make all stacks copyable, we will be able to shrink initial G stack significantly. At that point G struct size can be significant part of per-goroutine overhead. This change adds another 5 words to G. What do you think if we make a per-P cache of sudog's that is flushed during GC? Similar to what we have for defers. That would allow to implement selects and remove the sudog from G. https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) ep is a pointer into stack, you can't just enqueue it into sendq the stack can be shrunk while this goroutine is blocked, which will invalidate the pointer while we shrink stacks during STW only, I think we can just include a list of all sudog's that a G is waited on into G struct (for send it's at most 1 sudog, but for selects it's N sudog's); and then update them in copystack/shrinkstack also please add a test where a set of goroutines grow/shrink stack and do various chan operations; and another goroutine periodically calls GC we have potential problems with sudog, ep and selectdone, handling them is going to be tricky https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c File src/pkg/runtime/proc.c (right): https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... src/pkg/runtime/proc.c:409: runtime·ready_m() { (void) and { on the next line not Go yet :)
Sign in to reply to this message.
https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go File src/pkg/runtime/chan.go (right): https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... src/pkg/runtime/chan.go:194: mysg := &gp.chansudog I think we can use the same sudog (and the same cache) in chans, semaphores and in timer.
Sign in to reply to this message.
On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: > first round > > > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go > File src/pkg/runtime/chan.go (right): > > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode28 > src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M stack. > Is it bad? This function does not need to be in Go. > > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode30 > src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) > now that makechan is in Go can we just say: > c = new(hchan) > instead of: > c = (*hchan)(newobject(&hchanType)) > > then we don't need chaninit at all > > Excellent, that cleans up a lot. > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode81 > src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { > please move this to the bottom of the file > this is not the most important function in the file and this is Go now > > Done. > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode117 > src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, lock > *lock, reason string) { > move this to proc.go > > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode129 > src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason > string) { > move this to proc.go > > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode133 > src/pkg/runtime/chan.go:133: func goready(gp *g) { > move this to proc.go > > I'll wait on your change for these. > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode156 > src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, elem > unsafe.Pointer) { > this must be NOSPLIT as you call gogetcallerpc > > Looks like I haven't been doing that for the other callers of gogetcallerpc. A bunch are too big to be nosplit. It would be a shame to split them in two just to keep the race detector happy. Any other ideas? > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode194 > src/pkg/runtime/chan.go:194: mysg := &gp.chansudog > How do you want to handle select sudog's? > > When we make all stacks copyable, we will be able to shrink initial G > stack significantly. At that point G struct size can be significant part > of per-goroutine overhead. This change adds another 5 words to G. > > What do you think if we make a per-P cache of sudog's that is flushed > during GC? Similar to what we have for defers. That would allow to > implement selects and remove the sudog from G. > > I see, select needs one SudoG per case so having just one in the G isn't enough. Your change looks good then. > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/chan.go#newcode247 > src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) > ep is a pointer into stack, you can't just enqueue it into sendq > the stack can be shrunk while this goroutine is blocked, which will > invalidate the pointer > > while we shrink stacks during STW only, I think we can just include a > list of all sudog's that a G is waited on into G struct (for send it's > at most 1 sudog, but for selects it's N sudog's); and then update them > in copystack/shrinkstack > > Yes, we'll need to do that. The other option is to force ep to point to the heap, but I think that's probably worse. > also please add a test where a set of goroutines grow/shrink stack and > do various chan operations; and another goroutine periodically calls GC > we have potential problems with sudog, ep and selectdone, handling them > is going to be tricky > > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c > File src/pkg/runtime/proc.c (right): > > https://codereview.appspot.com/127460044/diff/80001/src/ > pkg/runtime/proc.c#newcode409 > src/pkg/runtime/proc.c:409: runtime·ready_m() { > (void) > and { on the next line > not Go yet :) > > Done. > https://codereview.appspot.com/127460044/ > Ok, I'll wait on your sudog cache change and then ping you for this.
Sign in to reply to this message.
On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> wrote: > > > > On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: >> >> first round >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go >> File src/pkg/runtime/chan.go (right): >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M stack. >> Is it bad? This function does not need to be in Go. >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) >> now that makechan is in Go can we just say: >> c = new(hchan) >> instead of: >> c = (*hchan)(newobject(&hchanType)) >> >> then we don't need chaninit at all >> > > Excellent, that cleans up a lot. > >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { >> please move this to the bottom of the file >> this is not the most important function in the file and this is Go now >> > > Done. > >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, lock >> *lock, reason string) { >> move this to proc.go >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason >> string) { >> move this to proc.go >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:133: func goready(gp *g) { >> move this to proc.go >> > > I'll wait on your change for these. > >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, elem >> unsafe.Pointer) { >> this must be NOSPLIT as you call gogetcallerpc >> > > Looks like I haven't been doing that for the other callers of gogetcallerpc. > A bunch are too big to be nosplit. It would be a shame to split them in two > just to keep the race detector happy. Any other ideas? Well, actually, on second though, I think we don't need NOSPLITs now in Go code. We used NOSPLIT to prevent seeing lessstack as return pc. But if we have only Go on g stacks and perfectly copyable stacks, then there must be no lessstack's. Lessstack can still appear in some weird cases (like reflect.call, panics or where we still use segments); this is not one of these cases, right? A dumb test for this and all new Go code could be to run runtime/race tests, capture all race reports and grep for lessstack. >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:194: mysg := &gp.chansudog >> How do you want to handle select sudog's? >> >> When we make all stacks copyable, we will be able to shrink initial G >> stack significantly. At that point G struct size can be significant part >> of per-goroutine overhead. This change adds another 5 words to G. >> >> What do you think if we make a per-P cache of sudog's that is flushed >> during GC? Similar to what we have for defers. That would allow to >> implement selects and remove the sudog from G. >> > > I see, select needs one SudoG per case so having just one in the G isn't > enough. Your change looks good then. > >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) >> ep is a pointer into stack, you can't just enqueue it into sendq >> the stack can be shrunk while this goroutine is blocked, which will >> invalidate the pointer >> >> while we shrink stacks during STW only, I think we can just include a >> list of all sudog's that a G is waited on into G struct (for send it's >> at most 1 sudog, but for selects it's N sudog's); and then update them >> in copystack/shrinkstack >> > > Yes, we'll need to do that. The other option is to force ep to point to the > heap, but I think that's probably worse. > >> >> also please add a test where a set of goroutines grow/shrink stack and >> do various chan operations; and another goroutine periodically calls GC >> we have potential problems with sudog, ep and selectdone, handling them >> is going to be tricky >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c >> File src/pkg/runtime/proc.c (right): >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... >> src/pkg/runtime/proc.c:409: runtime·ready_m() { >> (void) >> and { on the next line >> not Go yet :) >> > > Done. > >> >> https://codereview.appspot.com/127460044/ > > > Ok, I'll wait on your sudog cache change and then ping you for this.
Sign in to reply to this message.
Yes, once everything is copyable this problem goes away. But we're not there yet... On Thu, Aug 21, 2014 at 4:28 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> wrote: > > > > > > > > On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: > >> > >> first round > >> > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go > >> File src/pkg/runtime/chan.go (right): > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M stack. > >> Is it bad? This function does not need to be in Go. > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) > >> now that makechan is in Go can we just say: > >> c = new(hchan) > >> instead of: > >> c = (*hchan)(newobject(&hchanType)) > >> > >> then we don't need chaninit at all > >> > > > > Excellent, that cleans up a lot. > > > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { > >> please move this to the bottom of the file > >> this is not the most important function in the file and this is Go now > >> > > > > Done. > > > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, lock > >> *lock, reason string) { > >> move this to proc.go > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason > >> string) { > >> move this to proc.go > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:133: func goready(gp *g) { > >> move this to proc.go > >> > > > > I'll wait on your change for these. > > > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, elem > >> unsafe.Pointer) { > >> this must be NOSPLIT as you call gogetcallerpc > >> > > > > Looks like I haven't been doing that for the other callers of > gogetcallerpc. > > A bunch are too big to be nosplit. It would be a shame to split them in > two > > just to keep the race detector happy. Any other ideas? > > > Well, actually, on second though, I think we don't need NOSPLITs now > in Go code. We used NOSPLIT to prevent seeing lessstack as return pc. > But if we have only Go on g stacks and perfectly copyable stacks, then > there must be no lessstack's. > Lessstack can still appear in some weird cases (like reflect.call, > panics or where we still use segments); this is not one of these > cases, right? > > A dumb test for this and all new Go code could be to run runtime/race > tests, capture all race reports and grep for lessstack. > > > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:194: mysg := &gp.chansudog > >> How do you want to handle select sudog's? > >> > >> When we make all stacks copyable, we will be able to shrink initial G > >> stack significantly. At that point G struct size can be significant part > >> of per-goroutine overhead. This change adds another 5 words to G. > >> > >> What do you think if we make a per-P cache of sudog's that is flushed > >> during GC? Similar to what we have for defers. That would allow to > >> implement selects and remove the sudog from G. > >> > > > > I see, select needs one SudoG per case so having just one in the G isn't > > enough. Your change looks good then. > > > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) > >> ep is a pointer into stack, you can't just enqueue it into sendq > >> the stack can be shrunk while this goroutine is blocked, which will > >> invalidate the pointer > >> > >> while we shrink stacks during STW only, I think we can just include a > >> list of all sudog's that a G is waited on into G struct (for send it's > >> at most 1 sudog, but for selects it's N sudog's); and then update them > >> in copystack/shrinkstack > >> > > > > Yes, we'll need to do that. The other option is to force ep to point to > the > > heap, but I think that's probably worse. > > > >> > >> also please add a test where a set of goroutines grow/shrink stack and > >> do various chan operations; and another goroutine periodically calls GC > >> we have potential problems with sudog, ep and selectdone, handling them > >> is going to be tricky > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c > >> File src/pkg/runtime/proc.c (right): > >> > >> > >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... > >> src/pkg/runtime/proc.c:409: runtime·ready_m() { > >> (void) > >> and { on the next line > >> not Go yet :) > >> > > > > Done. > > > >> > >> https://codereview.appspot.com/127460044/ > > > > > > Ok, I'll wait on your sudog cache change and then ping you for this. >
Sign in to reply to this message.
On Thu, Aug 21, 2014 at 9:25 PM, Keith Randall <khr@google.com> wrote: > Yes, once everything is copyable this problem goes away. But we're not > there yet... But here we care only about immediate caller. An immediate caller of a copyable function cannot be lessstack, right? > On Thu, Aug 21, 2014 at 4:28 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >> >> On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> wrote: >> > >> > >> > >> > On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: >> >> >> >> first round >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go >> >> File src/pkg/runtime/chan.go (right): >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M stack. >> >> Is it bad? This function does not need to be in Go. >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) >> >> now that makechan is in Go can we just say: >> >> c = new(hchan) >> >> instead of: >> >> c = (*hchan)(newobject(&hchanType)) >> >> >> >> then we don't need chaninit at all >> >> >> > >> > Excellent, that cleans up a lot. >> > >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { >> >> please move this to the bottom of the file >> >> this is not the most important function in the file and this is Go now >> >> >> > >> > Done. >> > >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, lock >> >> *lock, reason string) { >> >> move this to proc.go >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason >> >> string) { >> >> move this to proc.go >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:133: func goready(gp *g) { >> >> move this to proc.go >> >> >> > >> > I'll wait on your change for these. >> > >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, elem >> >> unsafe.Pointer) { >> >> this must be NOSPLIT as you call gogetcallerpc >> >> >> > >> > Looks like I haven't been doing that for the other callers of >> > gogetcallerpc. >> > A bunch are too big to be nosplit. It would be a shame to split them in >> > two >> > just to keep the race detector happy. Any other ideas? >> >> >> Well, actually, on second though, I think we don't need NOSPLITs now >> in Go code. We used NOSPLIT to prevent seeing lessstack as return pc. >> But if we have only Go on g stacks and perfectly copyable stacks, then >> there must be no lessstack's. >> Lessstack can still appear in some weird cases (like reflect.call, >> panics or where we still use segments); this is not one of these >> cases, right? >> >> A dumb test for this and all new Go code could be to run runtime/race >> tests, capture all race reports and grep for lessstack. >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:194: mysg := &gp.chansudog >> >> How do you want to handle select sudog's? >> >> >> >> When we make all stacks copyable, we will be able to shrink initial G >> >> stack significantly. At that point G struct size can be significant >> >> part >> >> of per-goroutine overhead. This change adds another 5 words to G. >> >> >> >> What do you think if we make a per-P cache of sudog's that is flushed >> >> during GC? Similar to what we have for defers. That would allow to >> >> implement selects and remove the sudog from G. >> >> >> > >> > I see, select needs one SudoG per case so having just one in the G isn't >> > enough. Your change looks good then. >> > >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) >> >> ep is a pointer into stack, you can't just enqueue it into sendq >> >> the stack can be shrunk while this goroutine is blocked, which will >> >> invalidate the pointer >> >> >> >> while we shrink stacks during STW only, I think we can just include a >> >> list of all sudog's that a G is waited on into G struct (for send it's >> >> at most 1 sudog, but for selects it's N sudog's); and then update them >> >> in copystack/shrinkstack >> >> >> > >> > Yes, we'll need to do that. The other option is to force ep to point to >> > the >> > heap, but I think that's probably worse. >> > >> >> >> >> also please add a test where a set of goroutines grow/shrink stack and >> >> do various chan operations; and another goroutine periodically calls GC >> >> we have potential problems with sudog, ep and selectdone, handling them >> >> is going to be tricky >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c >> >> File src/pkg/runtime/proc.c (right): >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... >> >> src/pkg/runtime/proc.c:409: runtime·ready_m() { >> >> (void) >> >> and { on the next line >> >> not Go yet :) >> >> >> > >> > Done. >> > >> >> >> >> https://codereview.appspot.com/127460044/ >> > >> > >> > Ok, I'll wait on your sudog cache change and then ping you for this. > >
Sign in to reply to this message.
Yes, it can, if some other function higher up in the call chain (but still in the same segment) is not copyable. I don't think it happens very often though. Most of the noncopyable code at this point doesn't call back into Go. Maybe just Cgo? On Thu, Aug 21, 2014 at 10:27 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > On Thu, Aug 21, 2014 at 9:25 PM, Keith Randall <khr@google.com> wrote: > > Yes, once everything is copyable this problem goes away. But we're not > > there yet... > > But here we care only about immediate caller. An immediate caller of a > copyable function cannot be lessstack, right? > > > > On Thu, Aug 21, 2014 at 4:28 AM, Dmitry Vyukov <dvyukov@google.com> > wrote: > >> > >> On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> wrote: > >> > > >> > > >> > > >> > On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: > >> >> > >> >> first round > >> >> > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go > >> >> File src/pkg/runtime/chan.go (right): > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M > stack. > >> >> Is it bad? This function does not need to be in Go. > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) > >> >> now that makechan is in Go can we just say: > >> >> c = new(hchan) > >> >> instead of: > >> >> c = (*hchan)(newobject(&hchanType)) > >> >> > >> >> then we don't need chaninit at all > >> >> > >> > > >> > Excellent, that cleans up a lot. > >> > > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { > >> >> please move this to the bottom of the file > >> >> this is not the most important function in the file and this is Go > now > >> >> > >> > > >> > Done. > >> > > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, lock > >> >> *lock, reason string) { > >> >> move this to proc.go > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason > >> >> string) { > >> >> move this to proc.go > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:133: func goready(gp *g) { > >> >> move this to proc.go > >> >> > >> > > >> > I'll wait on your change for these. > >> > > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, > elem > >> >> unsafe.Pointer) { > >> >> this must be NOSPLIT as you call gogetcallerpc > >> >> > >> > > >> > Looks like I haven't been doing that for the other callers of > >> > gogetcallerpc. > >> > A bunch are too big to be nosplit. It would be a shame to split them > in > >> > two > >> > just to keep the race detector happy. Any other ideas? > >> > >> > >> Well, actually, on second though, I think we don't need NOSPLITs now > >> in Go code. We used NOSPLIT to prevent seeing lessstack as return pc. > >> But if we have only Go on g stacks and perfectly copyable stacks, then > >> there must be no lessstack's. > >> Lessstack can still appear in some weird cases (like reflect.call, > >> panics or where we still use segments); this is not one of these > >> cases, right? > >> > >> A dumb test for this and all new Go code could be to run runtime/race > >> tests, capture all race reports and grep for lessstack. > >> > >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:194: mysg := &gp.chansudog > >> >> How do you want to handle select sudog's? > >> >> > >> >> When we make all stacks copyable, we will be able to shrink initial G > >> >> stack significantly. At that point G struct size can be significant > >> >> part > >> >> of per-goroutine overhead. This change adds another 5 words to G. > >> >> > >> >> What do you think if we make a per-P cache of sudog's that is flushed > >> >> during GC? Similar to what we have for defers. That would allow to > >> >> implement selects and remove the sudog from G. > >> >> > >> > > >> > I see, select needs one SudoG per case so having just one in the G > isn't > >> > enough. Your change looks good then. > >> > > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) > >> >> ep is a pointer into stack, you can't just enqueue it into sendq > >> >> the stack can be shrunk while this goroutine is blocked, which will > >> >> invalidate the pointer > >> >> > >> >> while we shrink stacks during STW only, I think we can just include a > >> >> list of all sudog's that a G is waited on into G struct (for send > it's > >> >> at most 1 sudog, but for selects it's N sudog's); and then update > them > >> >> in copystack/shrinkstack > >> >> > >> > > >> > Yes, we'll need to do that. The other option is to force ep to point > to > >> > the > >> > heap, but I think that's probably worse. > >> > > >> >> > >> >> also please add a test where a set of goroutines grow/shrink stack > and > >> >> do various chan operations; and another goroutine periodically calls > GC > >> >> we have potential problems with sudog, ep and selectdone, handling > them > >> >> is going to be tricky > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c > >> >> File src/pkg/runtime/proc.c (right): > >> >> > >> >> > >> >> > >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... > >> >> src/pkg/runtime/proc.c:409: runtime·ready_m() { > >> >> (void) > >> >> and { on the next line > >> >> not Go yet :) > >> >> > >> > > >> > Done. > >> > > >> >> > >> >> https://codereview.appspot.com/127460044/ > >> > > >> > > >> > Ok, I'll wait on your sudog cache change and then ping you for this. > > > > >
Sign in to reply to this message.
Ah, OK, I understand how it can happen. So, first, there are cases where normal code needs pc -- e.g. select cases; we have to split them. For the race detector we can have something special. runtime.callers knows how to skip lessstack; but it calls gentraceback, so it's quite expensive (at least for some of the functions) We can have function func racecallerpc(argp unsafe.Pointer) uintptr call it as: func chansend(c ...) { racerelease(..., racecallerpc(unsafe.Pointer(&c))) ... } Then, in non-race build it is no-op. In race build, it first calls gogetcallerpc with the argp passed from above, if it returns lessstack it resorts to runtime.callers(2, pcs, 1). When we convert everything to Go, it will be simple to check that the lessstack case never happens. And then we can remove the runtime.callers fallback. It actually will be faster than what we have now, because now we call getcallerpc in normal non-race build. On Thu, Aug 21, 2014 at 9:31 PM, Keith Randall <khr@google.com> wrote: > Yes, it can, if some other function higher up in the call chain (but still > in the same segment) is not copyable. > > I don't think it happens very often though. Most of the noncopyable code at > this point doesn't call back into Go. Maybe just Cgo? > > > On Thu, Aug 21, 2014 at 10:27 AM, Dmitry Vyukov <dvyukov@google.com> wrote: >> >> On Thu, Aug 21, 2014 at 9:25 PM, Keith Randall <khr@google.com> wrote: >> > Yes, once everything is copyable this problem goes away. But we're not >> > there yet... >> >> But here we care only about immediate caller. An immediate caller of a >> copyable function cannot be lessstack, right? >> >> >> > On Thu, Aug 21, 2014 at 4:28 AM, Dmitry Vyukov <dvyukov@google.com> >> > wrote: >> >> >> >> On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> wrote: >> >> > >> >> > >> >> > >> >> > On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: >> >> >> >> >> >> first round >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go >> >> >> File src/pkg/runtime/chan.go (right): >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M >> >> >> stack. >> >> >> Is it bad? This function does not need to be in Go. >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) >> >> >> now that makechan is in Go can we just say: >> >> >> c = new(hchan) >> >> >> instead of: >> >> >> c = (*hchan)(newobject(&hchanType)) >> >> >> >> >> >> then we don't need chaninit at all >> >> >> >> >> > >> >> > Excellent, that cleans up a lot. >> >> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { >> >> >> please move this to the bottom of the file >> >> >> this is not the most important function in the file and this is Go >> >> >> now >> >> >> >> >> > >> >> > Done. >> >> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, >> >> >> lock >> >> >> *lock, reason string) { >> >> >> move this to proc.go >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason >> >> >> string) { >> >> >> move this to proc.go >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:133: func goready(gp *g) { >> >> >> move this to proc.go >> >> >> >> >> > >> >> > I'll wait on your change for these. >> >> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, >> >> >> elem >> >> >> unsafe.Pointer) { >> >> >> this must be NOSPLIT as you call gogetcallerpc >> >> >> >> >> > >> >> > Looks like I haven't been doing that for the other callers of >> >> > gogetcallerpc. >> >> > A bunch are too big to be nosplit. It would be a shame to split them >> >> > in >> >> > two >> >> > just to keep the race detector happy. Any other ideas? >> >> >> >> >> >> Well, actually, on second though, I think we don't need NOSPLITs now >> >> in Go code. We used NOSPLIT to prevent seeing lessstack as return pc. >> >> But if we have only Go on g stacks and perfectly copyable stacks, then >> >> there must be no lessstack's. >> >> Lessstack can still appear in some weird cases (like reflect.call, >> >> panics or where we still use segments); this is not one of these >> >> cases, right? >> >> >> >> A dumb test for this and all new Go code could be to run runtime/race >> >> tests, capture all race reports and grep for lessstack. >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:194: mysg := &gp.chansudog >> >> >> How do you want to handle select sudog's? >> >> >> >> >> >> When we make all stacks copyable, we will be able to shrink initial >> >> >> G >> >> >> stack significantly. At that point G struct size can be significant >> >> >> part >> >> >> of per-goroutine overhead. This change adds another 5 words to G. >> >> >> >> >> >> What do you think if we make a per-P cache of sudog's that is >> >> >> flushed >> >> >> during GC? Similar to what we have for defers. That would allow to >> >> >> implement selects and remove the sudog from G. >> >> >> >> >> > >> >> > I see, select needs one SudoG per case so having just one in the G >> >> > isn't >> >> > enough. Your change looks good then. >> >> > >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... >> >> >> src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) >> >> >> ep is a pointer into stack, you can't just enqueue it into sendq >> >> >> the stack can be shrunk while this goroutine is blocked, which will >> >> >> invalidate the pointer >> >> >> >> >> >> while we shrink stacks during STW only, I think we can just include >> >> >> a >> >> >> list of all sudog's that a G is waited on into G struct (for send >> >> >> it's >> >> >> at most 1 sudog, but for selects it's N sudog's); and then update >> >> >> them >> >> >> in copystack/shrinkstack >> >> >> >> >> > >> >> > Yes, we'll need to do that. The other option is to force ep to point >> >> > to >> >> > the >> >> > heap, but I think that's probably worse. >> >> > >> >> >> >> >> >> also please add a test where a set of goroutines grow/shrink stack >> >> >> and >> >> >> do various chan operations; and another goroutine periodically calls >> >> >> GC >> >> >> we have potential problems with sudog, ep and selectdone, handling >> >> >> them >> >> >> is going to be tricky >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c >> >> >> File src/pkg/runtime/proc.c (right): >> >> >> >> >> >> >> >> >> >> >> >> >> >> >> https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... >> >> >> src/pkg/runtime/proc.c:409: runtime·ready_m() { >> >> >> (void) >> >> >> and { on the next line >> >> >> not Go yet :) >> >> >> >> >> > >> >> > Done. >> >> > >> >> >> >> >> >> https://codereview.appspot.com/127460044/ >> >> > >> >> > >> >> > Ok, I'll wait on your sudog cache change and then ping you for this. >> > >> > > >
Sign in to reply to this message.
Sounds good. On Thu, Aug 21, 2014 at 10:44 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > Ah, OK, I understand how it can happen. > > So, first, there are cases where normal code needs pc -- e.g. select > cases; we have to split them. > > For the race detector we can have something special. > runtime.callers knows how to skip lessstack; but it calls > gentraceback, so it's quite expensive (at least for some of the > functions) > > We can have function > > func racecallerpc(argp unsafe.Pointer) uintptr > > call it as: > > func chansend(c ...) { > racerelease(..., racecallerpc(unsafe.Pointer(&c))) > ... > } > > Then, in non-race build it is no-op. > In race build, it first calls gogetcallerpc with the argp passed from > above, if it returns lessstack it resorts to runtime.callers(2, pcs, > 1). > > When we convert everything to Go, it will be simple to check that the > lessstack case never happens. And then we can remove the > runtime.callers fallback. > > It actually will be faster than what we have now, because now we call > getcallerpc in normal non-race build. > > > > > > On Thu, Aug 21, 2014 at 9:31 PM, Keith Randall <khr@google.com> wrote: > > Yes, it can, if some other function higher up in the call chain (but > still > > in the same segment) is not copyable. > > > > I don't think it happens very often though. Most of the noncopyable > code at > > this point doesn't call back into Go. Maybe just Cgo? > > > > > > On Thu, Aug 21, 2014 at 10:27 AM, Dmitry Vyukov <dvyukov@google.com> > wrote: > >> > >> On Thu, Aug 21, 2014 at 9:25 PM, Keith Randall <khr@google.com> wrote: > >> > Yes, once everything is copyable this problem goes away. But we're > not > >> > there yet... > >> > >> But here we care only about immediate caller. An immediate caller of a > >> copyable function cannot be lessstack, right? > >> > >> > >> > On Thu, Aug 21, 2014 at 4:28 AM, Dmitry Vyukov <dvyukov@google.com> > >> > wrote: > >> >> > >> >> On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> > wrote: > >> >> > > >> >> > > >> >> > > >> >> > On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: > >> >> >> > >> >> >> first round > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go > >> >> >> File src/pkg/runtime/chan.go (right): > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:28: // Note: this Go code runs on the M > >> >> >> stack. > >> >> >> Is it bad? This function does not need to be in Go. > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:30: hchanType.size = uint(hchanSize) > >> >> >> now that makechan is in Go can we just say: > >> >> >> c = new(hchan) > >> >> >> instead of: > >> >> >> c = (*hchan)(newobject(&hchanType)) > >> >> >> > >> >> >> then we don't need chaninit at all > >> >> >> > >> >> > > >> >> > Excellent, that cleans up a lot. > >> >> > > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:81: func goenqueue(q *waitq, sgp *sudog) { > >> >> >> please move this to the bottom of the file > >> >> >> this is not the most important function in the file and this is Go > >> >> >> now > >> >> >> > >> >> > > >> >> > Done. > >> >> > > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:117: func gopark(unlockf unsafe.Pointer, > >> >> >> lock > >> >> >> *lock, reason string) { > >> >> >> move this to proc.go > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:129: func goparkunlock(lock *lock, reason > >> >> >> string) { > >> >> >> move this to proc.go > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:133: func goready(gp *g) { > >> >> >> move this to proc.go > >> >> >> > >> >> > > >> >> > I'll wait on your change for these. > >> >> > > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:156: func chansend1(t *chantype, c *hchan, > >> >> >> elem > >> >> >> unsafe.Pointer) { > >> >> >> this must be NOSPLIT as you call gogetcallerpc > >> >> >> > >> >> > > >> >> > Looks like I haven't been doing that for the other callers of > >> >> > gogetcallerpc. > >> >> > A bunch are too big to be nosplit. It would be a shame to split > them > >> >> > in > >> >> > two > >> >> > just to keep the race detector happy. Any other ideas? > >> >> > >> >> > >> >> Well, actually, on second though, I think we don't need NOSPLITs now > >> >> in Go code. We used NOSPLIT to prevent seeing lessstack as return pc. > >> >> But if we have only Go on g stacks and perfectly copyable stacks, > then > >> >> there must be no lessstack's. > >> >> Lessstack can still appear in some weird cases (like reflect.call, > >> >> panics or where we still use segments); this is not one of these > >> >> cases, right? > >> >> > >> >> A dumb test for this and all new Go code could be to run runtime/race > >> >> tests, capture all race reports and grep for lessstack. > >> >> > >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:194: mysg := &gp.chansudog > >> >> >> How do you want to handle select sudog's? > >> >> >> > >> >> >> When we make all stacks copyable, we will be able to shrink > initial > >> >> >> G > >> >> >> stack significantly. At that point G struct size can be > significant > >> >> >> part > >> >> >> of per-goroutine overhead. This change adds another 5 words to G. > >> >> >> > >> >> >> What do you think if we make a per-P cache of sudog's that is > >> >> >> flushed > >> >> >> during GC? Similar to what we have for defers. That would allow to > >> >> >> implement selects and remove the sudog from G. > >> >> >> > >> >> > > >> >> > I see, select needs one SudoG per case so having just one in the G > >> >> > isn't > >> >> > enough. Your change looks good then. > >> >> > > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/chan.go#n... > >> >> >> src/pkg/runtime/chan.go:247: mysg.elem = (*uint8)(ep) > >> >> >> ep is a pointer into stack, you can't just enqueue it into sendq > >> >> >> the stack can be shrunk while this goroutine is blocked, which > will > >> >> >> invalidate the pointer > >> >> >> > >> >> >> while we shrink stacks during STW only, I think we can just > include > >> >> >> a > >> >> >> list of all sudog's that a G is waited on into G struct (for send > >> >> >> it's > >> >> >> at most 1 sudog, but for selects it's N sudog's); and then update > >> >> >> them > >> >> >> in copystack/shrinkstack > >> >> >> > >> >> > > >> >> > Yes, we'll need to do that. The other option is to force ep to > point > >> >> > to > >> >> > the > >> >> > heap, but I think that's probably worse. > >> >> > > >> >> >> > >> >> >> also please add a test where a set of goroutines grow/shrink stack > >> >> >> and > >> >> >> do various chan operations; and another goroutine periodically > calls > >> >> >> GC > >> >> >> we have potential problems with sudog, ep and selectdone, handling > >> >> >> them > >> >> >> is going to be tricky > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c > >> >> >> File src/pkg/runtime/proc.c (right): > >> >> >> > >> >> >> > >> >> >> > >> >> >> > >> >> >> > https://codereview.appspot.com/127460044/diff/80001/src/pkg/runtime/proc.c#ne... > >> >> >> src/pkg/runtime/proc.c:409: runtime·ready_m() { > >> >> >> (void) > >> >> >> and { on the next line > >> >> >> not Go yet :) > >> >> >> > >> >> > > >> >> > Done. > >> >> > > >> >> >> > >> >> >> https://codereview.appspot.com/127460044/ > >> >> > > >> >> > > >> >> > Ok, I'll wait on your sudog cache change and then ping you for > this. > >> > > >> > > > > > >
Sign in to reply to this message.
PTAL. Everything is fixed up. I'm having second thoughts about keeping these SudoG elsewhere. I think a simpler solution is to leave the SudoG on the stack and change the stack shrinking code to never copy - always shrink in place. Then we just have to make sure we don't do anything which might grow the stack between when enqueue() is called and when we park the thread. Looking at chan.goc it seems doable, we just need enqueue, parkunlock, and park to be nosplit. To make shrinking never copy we could use span splitting for large stacks (like we used to) and a buddy allocator for the small stacks. I have a prototype implementation for that but I ended up not using it because the tcmalloc-style allocator was easier. Let me know what you think.
Sign in to reply to this message.
https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go File src/pkg/runtime/chan.go (right): https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go#... src/pkg/runtime/chan.go:10: import ( remove () we can't import anything except "unsafe" in runtime anyway https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go#... src/pkg/runtime/chan.go:83: var t0, t1 int64 move definition of t0 just above: t0 = gocputicks() and definition of t1 to: // wait for some space to write our data https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go#... src/pkg/runtime/chan.go:154: mysg.elem = (*uint8)(ep) // TODO: this could store a pointer into the stack into the heap. Fix the stack copier. you've already fixed stack copier, right? https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go#... src/pkg/runtime/chan.go:170: if t0 > 0 { this must be mysg.releasetime > 0 https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go#... src/pkg/runtime/chan.go:235: if t0 != 0 { t1 > 0 https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.go#... src/pkg/runtime/chan.go:241: func goenqueue(q *waitq, sgp *sudog) { We now don't have static functions; and prefixing everything with 'go' does not look like a good solution long-term, and it does not save us from name collisions in Go code anyway. We need either give such functions "subsystem" prefixes -- chan in this case; or start using methods. I've hit the same problem in time.goc. Methods are probably more idiomatic. enqueue/dequeue can be methods on (q *waitq). https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.goc File src/pkg/runtime/chan.goc (right): https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan.goc... src/pkg/runtime/chan.goc:36: chansend(ChanType *t, Hchan *c, byte *ep, bool block, void *pc) delete chansend https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan_tes... File src/pkg/runtime/chan_test.go (right): https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/chan_tes... src/pkg/runtime/chan_test.go:450: go func() { Does this need to be a separate goroutine? https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/127460044/diff/160001/src/pkg/runtime/stack.c#... src/pkg/runtime/stack.c:768: if(adjinfo->oldstk <= e && e < adjinfo->oldbase) { drop {}
Sign in to reply to this message.
On 2014/08/22 00:13:58, khr wrote: > PTAL. > > Everything is fixed up. > > I'm having second thoughts about keeping these SudoG elsewhere. I think a > simpler solution is to leave the SudoG on the stack and change the stack > shrinking code to never copy - always shrink in place. Then we just have to > make sure we don't do anything which might grow the stack between when enqueue() > is called and when we park the thread. Looking at chan.goc it seems doable, we > just need enqueue, parkunlock, and park to be nosplit. > > To make shrinking never copy we could use span splitting for large stacks (like > we used to) and a buddy allocator for the small stacks. I have a prototype > implementation for that but I ended up not using it because the tcmalloc-style > allocator was easier. > > Let me know what you think. Allocating things on stack and shrinking w/o copying sounds good to me. Will it be possible to grow w/o copying in some cases? However, I never used buddy allocator so I don't know what is its performance/memory overhead/fragmentation/scalability. We also need to make sure that it works for all use cases, which are at least chans, selects, 2 types of semaphores and timers.
Sign in to reply to this message.
On Fri, Aug 22, 2014 at 2:30 AM, <dvyukov@google.com> wrote: > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go > File src/pkg/runtime/chan.go (right): > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go#newcode10 > src/pkg/runtime/chan.go:10: import ( > remove () > we can't import anything except "unsafe" in runtime anyway > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go#newcode83 > src/pkg/runtime/chan.go:83: var t0, t1 int64 > move definition of t0 just above: > t0 = gocputicks() > > and definition of t1 to: > // wait for some space to write our data > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go#newcode154 > src/pkg/runtime/chan.go:154: mysg.elem = (*uint8)(ep) // TODO: this > could store a pointer into the stack into the heap. Fix the stack > copier. > you've already fixed stack copier, right? > Yes, I did. Removed TODO. > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go#newcode170 > src/pkg/runtime/chan.go:170: if t0 > 0 { > this must be mysg.releasetime > 0 > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go#newcode235 > src/pkg/runtime/chan.go:235: if t0 != 0 { > t1 > 0 > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.go#newcode241 > src/pkg/runtime/chan.go:241: func goenqueue(q *waitq, sgp *sudog) { > We now don't have static functions; and prefixing everything with 'go' > does not look like a good solution long-term, and it does not save us > from name collisions in Go code anyway. > > We need either give such functions "subsystem" prefixes -- chan in this > case; or start using methods. > I've hit the same problem in time.goc. > Methods are probably more idiomatic. enqueue/dequeue can be methods on > (q *waitq). > > I made them methods. > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.goc > File src/pkg/runtime/chan.goc (right): > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan.goc#newcode36 > src/pkg/runtime/chan.goc:36: chansend(ChanType *t, Hchan *c, byte *ep, > bool block, void *pc) > delete chansend > > It is still used by some code in chan.goc. Don't worry, it will go away when everything gets translated. > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan_test.go > File src/pkg/runtime/chan_test.go (right): > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/chan_test.go#newcode450 > src/pkg/runtime/chan_test.go:450: go func() { > Does this need to be a separate goroutine? > > No, it can run in the main test goroutine. Fixed. > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/stack.c > File src/pkg/runtime/stack.c (right): > > https://codereview.appspot.com/127460044/diff/160001/src/ > pkg/runtime/stack.c#newcode768 > src/pkg/runtime/stack.c:768: if(adjinfo->oldstk <= e && e < > adjinfo->oldbase) { > drop {} > > https://codereview.appspot.com/127460044/ >
Sign in to reply to this message.
LGTM
Sign in to reply to this message.
*** Submitted as https://code.google.com/p/go/source/detail?r=f0f0297e80bc *** runtime: convert channel operations to Go, part 1 (chansend1). LGTM=dvyukov R=dvyukov, khr CC=golang-codereviews https://codereview.appspot.com/127460044 Committer: Dmitriy Vyukov <dvyukov@google.com> https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c File src/pkg/runtime/stack.c (right): https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c#... src/pkg/runtime/stack.c:1091: runtime·printf("shrinking stack %D->%D\n", oldsize, newsize); changed this to runtime·printf("shrinking stack %D->%D\n", (uint64)oldsize, (uint64)newsize);
Sign in to reply to this message.
On 2014/08/24 08:31:08, dvyukov wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=f0f0297e80bc *** > > runtime: convert channel operations to Go, part 1 (chansend1). > > LGTM=dvyukov > R=dvyukov, khr > CC=golang-codereviews > https://codereview.appspot.com/127460044 > > Committer: Dmitriy Vyukov <mailto:dvyukov@google.com> > > https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c > File src/pkg/runtime/stack.c (right): > > https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c#... > src/pkg/runtime/stack.c:1091: runtime·printf("shrinking stack %D->%D\n", > oldsize, newsize); > changed this to > runtime·printf("shrinking stack %D->%D\n", (uint64)oldsize, (uint64)newsize); Keith, I've submitted this because I need the race instrumentation from this change, and so that you don't need to resync if we submit my chan changes.
Sign in to reply to this message.
This CL appears to have broken the android-arm-crawshaw builder. See http://build.golang.org/log/d1d4c48a08d4e8d08946da6c3a88c17a39469809
Sign in to reply to this message.
Dmitry, I think the arm portions of this change aren't quite right, it broke all the arm builders ok runtime/debug 0.030s unexpected fault address 0x0 fatal error: fault [signal 0x7 code=0x1 addr=0x0 pc=0x3f998] goroutine 29 [running]: runtime.throw(0x1f7bae) /usr/local/go/src/pkg/runtime/panic.c:510 +0x60 fp=0x10443ef4 sp=0x10443ee8 runtime.sigpanic() /usr/local/go/src/pkg/runtime/os_linux.c:233 +0x1b8 fp=0x10443f00 sp=0x10443ef4 _addv(0x1, 0x4556a7c0, 0xaafb, 0xe41b0700, 0x0) /usr/local/go/src/pkg/runtime/vlrt_arm.c:53 +0x10 fp=0x10443f04 sp=0x10443f04 runtime.cputicks(0x1) /usr/local/go/src/pkg/runtime/os_linux_arm.c:79 +0x5c fp=0x10443f2c sp=0x10443f04 runtime.gocputicks(0x46474, 0x33e6c) /usr/local/go/src/pkg/runtime/asm_arm.s:652 +0x10 fp=0x10443f34 sp=0x10443f2c runtime.chansend(0x117fa8, 0x10444120, 0x10443fc4, 0x1, 0x122cc, 0x5) /usr/local/go/src/pkg/runtime/chan.go:101 +0xc0 fp=0x10443f98 sp=0x10443f34 created by runtime/pprof_test.blockChanRecv /export/builder/workspace/linux-arm-arm5-f0f0297e80bc/go/src/pkg/runtime/pprof/pprof_test.go:347 +0x98 On Sun, Aug 24, 2014 at 6:32 PM, dvyukov via golang-codereviews <golang-codereviews@googlegroups.com> wrote: > On 2014/08/24 08:31:08, dvyukov wrote: >> >> *** Submitted as > > https://code.google.com/p/go/source/detail?r=f0f0297e80bc *** > >> runtime: convert channel operations to Go, part 1 (chansend1). > > >> LGTM=dvyukov >> R=dvyukov, khr >> CC=golang-codereviews >> https://codereview.appspot.com/127460044 > > >> Committer: Dmitriy Vyukov <mailto:dvyukov@google.com> > > > > https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c >> >> File src/pkg/runtime/stack.c (right): > > > > https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c#... >> >> src/pkg/runtime/stack.c:1091: runtime·printf("shrinking stack > > %D->%D\n", >> >> oldsize, newsize); >> changed this to >> runtime·printf("shrinking stack %D->%D\n", (uint64)oldsize, > > (uint64)newsize); > > > Keith, I've submitted this because I need the race instrumentation from > this change, and so that you don't need to resync if we submit my chan > changes. > > > > https://codereview.appspot.com/127460044/ > > -- > You received this message because you are subscribed to the Google Groups > "golang-codereviews" group. > To unsubscribe from this group and stop receiving emails from it, send an > email to golang-codereviews+unsubscribe@googlegroups.com. > For more options, visit https://groups.google.com/d/optout.
Sign in to reply to this message.
Any ideas as to what is wrong in arm code? On Sun, Aug 24, 2014 at 3:09 PM, Dave Cheney <dave@cheney.net> wrote: > Dmitry, I think the arm portions of this change aren't quite right, it > broke all the arm builders > > ok runtime/debug 0.030s > unexpected fault address 0x0 > fatal error: fault > [signal 0x7 code=0x1 addr=0x0 pc=0x3f998] > > goroutine 29 [running]: > runtime.throw(0x1f7bae) > /usr/local/go/src/pkg/runtime/panic.c:510 +0x60 fp=0x10443ef4 sp=0x10443ee8 > runtime.sigpanic() > /usr/local/go/src/pkg/runtime/os_linux.c:233 +0x1b8 fp=0x10443f00 sp=0x10443ef4 > _addv(0x1, 0x4556a7c0, 0xaafb, 0xe41b0700, 0x0) > /usr/local/go/src/pkg/runtime/vlrt_arm.c:53 +0x10 fp=0x10443f04 sp=0x10443f04 > runtime.cputicks(0x1) > /usr/local/go/src/pkg/runtime/os_linux_arm.c:79 +0x5c fp=0x10443f2c > sp=0x10443f04 > runtime.gocputicks(0x46474, 0x33e6c) > /usr/local/go/src/pkg/runtime/asm_arm.s:652 +0x10 fp=0x10443f34 sp=0x10443f2c > runtime.chansend(0x117fa8, 0x10444120, 0x10443fc4, 0x1, 0x122cc, 0x5) > /usr/local/go/src/pkg/runtime/chan.go:101 +0xc0 fp=0x10443f98 sp=0x10443f34 > created by runtime/pprof_test.blockChanRecv > /export/builder/workspace/linux-arm-arm5-f0f0297e80bc/go/src/pkg/runtime/pprof/pprof_test.go:347 > +0x98 > > On Sun, Aug 24, 2014 at 6:32 PM, dvyukov via golang-codereviews > <golang-codereviews@googlegroups.com> wrote: >> On 2014/08/24 08:31:08, dvyukov wrote: >>> >>> *** Submitted as >> >> https://code.google.com/p/go/source/detail?r=f0f0297e80bc *** >> >>> runtime: convert channel operations to Go, part 1 (chansend1). >> >> >>> LGTM=dvyukov >>> R=dvyukov, khr >>> CC=golang-codereviews >>> https://codereview.appspot.com/127460044 >> >> >>> Committer: Dmitriy Vyukov <mailto:dvyukov@google.com> >> >> >> >> https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c >>> >>> File src/pkg/runtime/stack.c (right): >> >> >> >> https://codereview.appspot.com/127460044/diff/180001/src/pkg/runtime/stack.c#... >>> >>> src/pkg/runtime/stack.c:1091: runtime·printf("shrinking stack >> >> %D->%D\n", >>> >>> oldsize, newsize); >>> changed this to >>> runtime·printf("shrinking stack %D->%D\n", (uint64)oldsize, >> >> (uint64)newsize); >> >> >> Keith, I've submitted this because I need the race instrumentation from >> this change, and so that you don't need to resync if we submit my chan >> changes. >> >> >> >> https://codereview.appspot.com/127460044/ >> >> -- >> You received this message because you are subscribed to the Google Groups >> "golang-codereviews" group. >> To unsubscribe from this group and stop receiving emails from it, send an >> email to golang-codereviews+unsubscribe@googlegroups.com. >> For more options, visit https://groups.google.com/d/optout.
Sign in to reply to this message.
|