Rietveld Code Review Tool
Help | Bug tracker | Discussion group | Source code | Sign in
(10)

Issue 127460044: code review 127460044: runtime: convert channel operations to Go, part 1 (chan... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
9 years, 7 months ago by khr
Modified:
9 years, 7 months ago
Reviewers:
gobot, dfc, dvyukov
CC:
khr1, golang-codereviews
Visibility:
Public.

Description

runtime: 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
Unified diffs Side-by-side diffs Delta from patch set Stats (+378 lines, -95 lines) Patch
M src/cmd/api/goapi.go View 1 2 3 4 5 6 7 8 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/gc/select.c View 1 2 3 4 5 6 7 8 9 1 chunk +1 line, -0 lines 0 comments Download
M src/pkg/reflect/asm_386.s View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/reflect/asm_amd64.s View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/reflect/asm_amd64p32.s View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/reflect/asm_arm.s View 1 1 chunk +2 lines, -0 lines 0 comments Download
M src/pkg/runtime/chan.h View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -13 lines 0 comments Download
A src/pkg/runtime/chan.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +278 lines, -0 lines 0 comments Download
M src/pkg/runtime/chan.goc View 1 2 3 2 chunks +0 lines, -77 lines 0 comments Download
M src/pkg/runtime/chan_test.go View 1 2 3 4 5 6 7 8 9 10 1 chunk +32 lines, -0 lines 0 comments Download
M src/pkg/runtime/mprof.goc View 1 2 3 4 5 6 7 8 2 chunks +10 lines, -1 line 0 comments Download
M src/pkg/runtime/pprof/pprof_test.go View 1 2 1 chunk +1 line, -1 line 0 comments Download
M src/pkg/runtime/proc.c View 1 2 3 4 5 6 7 8 1 chunk +0 lines, -1 line 0 comments Download
M src/pkg/runtime/runtime.h View 1 2 3 4 5 6 7 8 9 4 chunks +14 lines, -1 line 0 comments Download
M src/pkg/runtime/stack.c View 1 2 3 4 5 6 7 8 9 10 4 chunks +21 lines, -1 line 1 comment Download
M src/pkg/runtime/stubs.go View 1 2 3 4 5 6 7 8 1 chunk +12 lines, -0 lines 0 comments Download

Messages

Total messages: 20
khr
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/
9 years, 7 months ago (2014-08-18 23:52:06 UTC) #1
dvyukov
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 ...
9 years, 7 months ago (2014-08-20 14:02:12 UTC) #2
dvyukov
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#newcode194 src/pkg/runtime/chan.go:194: mysg := &gp.chansudog I think we can use the ...
9 years, 7 months ago (2014-08-20 15:54:12 UTC) #3
khr1
On Wed, Aug 20, 2014 at 7:02 AM, <dvyukov@google.com> wrote: > first round > > ...
9 years, 7 months ago (2014-08-20 21:07:03 UTC) #4
dvyukov
On Thu, Aug 21, 2014 at 1:07 AM, Keith Randall <khr@google.com> wrote: > > > ...
9 years, 7 months ago (2014-08-21 11:28:26 UTC) #5
khr1
Yes, once everything is copyable this problem goes away. But we're not there yet... On ...
9 years, 7 months ago (2014-08-21 17:25:21 UTC) #6
dvyukov
On Thu, Aug 21, 2014 at 9:25 PM, Keith Randall <khr@google.com> wrote: > Yes, once ...
9 years, 7 months ago (2014-08-21 17:27:36 UTC) #7
khr1
Yes, it can, if some other function higher up in the call chain (but still ...
9 years, 7 months ago (2014-08-21 17:31:59 UTC) #8
dvyukov
Ah, OK, I understand how it can happen. So, first, there are cases where normal ...
9 years, 7 months ago (2014-08-21 17:44:43 UTC) #9
khr1
Sounds good. On Thu, Aug 21, 2014 at 10:44 AM, Dmitry Vyukov <dvyukov@google.com> wrote: > ...
9 years, 7 months ago (2014-08-21 18:03:23 UTC) #10
khr
PTAL. Everything is fixed up. I'm having second thoughts about keeping these SudoG elsewhere. I ...
9 years, 7 months ago (2014-08-22 00:13:58 UTC) #11
dvyukov
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 ...
9 years, 7 months ago (2014-08-22 09:30:51 UTC) #12
dvyukov
On 2014/08/22 00:13:58, khr wrote: > PTAL. > > Everything is fixed up. > > ...
9 years, 7 months ago (2014-08-22 09:36:02 UTC) #13
khr1
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 ...
9 years, 7 months ago (2014-08-22 20:03:03 UTC) #14
dvyukov
LGTM
9 years, 7 months ago (2014-08-22 20:26:22 UTC) #15
dvyukov
*** Submitted as https://code.google.com/p/go/source/detail?r=f0f0297e80bc *** runtime: convert channel operations to Go, part 1 (chansend1). LGTM=dvyukov ...
9 years, 7 months ago (2014-08-24 08:31:08 UTC) #16
dvyukov
On 2014/08/24 08:31:08, dvyukov wrote: > *** Submitted as https://code.google.com/p/go/source/detail?r=f0f0297e80bc *** > > runtime: convert ...
9 years, 7 months ago (2014-08-24 08:32:21 UTC) #17
gobot
This CL appears to have broken the android-arm-crawshaw builder. See http://build.golang.org/log/d1d4c48a08d4e8d08946da6c3a88c17a39469809
9 years, 7 months ago (2014-08-24 08:35:45 UTC) #18
dfc
Dmitry, I think the arm portions of this change aren't quite right, it broke all ...
9 years, 7 months ago (2014-08-24 11:09:11 UTC) #19
dvyukov
9 years, 7 months ago (2014-08-24 16:15:07 UTC) #20
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.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld f62528b