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

Issue 129090043: code review 129090043: cmd/gc, runtime: refactor interface inlining decision i... (Closed)

Can't Edit
Can't Publish+Mail
Start Review
Created:
3 years, 1 month ago by rsc
Modified:
3 years ago
Reviewers:
gobot, iant
CC:
golang-codereviews, iant, r
Visibility:
Public.

Description

cmd/gc, runtime: refactor interface inlining decision into compiler We need to change the interface value representation for concurrent garbage collection, so that there is no ambiguity about whether the data word holds a pointer or scalar. This CL does NOT make any representation changes. Instead, it removes representation assumptions from various pieces of code throughout the tree. The isdirectiface function in cmd/gc/subr.c is now the only place that decides that policy. The policy propagates out from there in the reflect metadata, as a new flag in the internal kind value. A follow-up CL will change the representation by changing the isdirectiface function. If that CL causes problems, it will be easy to roll back. Update issue 8405.

Patch Set 1 #

Patch Set 2 : diff -r 88a093108a9d774937ff8af61006f87ce20b0651 https://code.google.com/p/go/ #

Patch Set 3 : diff -r 88a093108a9d774937ff8af61006f87ce20b0651 https://code.google.com/p/go/ #

Total comments: 6

Patch Set 4 : diff -r 5856eae43c269fc32cdf2cfca0c9412ee4a5969c https://code.google.com/p/go/ #

Unified diffs Side-by-side diffs Delta from patch set Stats (+230 lines, -105 lines) Patch
M src/cmd/gc/go.h View 1 1 chunk +1 line, -0 lines 0 comments Download
M src/cmd/gc/reflect.c View 1 2 chunks +3 lines, -1 line 0 comments Download
M src/cmd/gc/subr.c View 1 1 chunk +39 lines, -0 lines 0 comments Download
M src/cmd/gc/walk.c View 1 2 chunks +2 lines, -5 lines 0 comments Download
M src/pkg/database/sql/convert_test.go View 1 2 chunks +24 lines, -13 lines 0 comments Download
M src/pkg/reflect/all_test.go View 1 1 chunk +3 lines, -0 lines 0 comments Download
M src/pkg/reflect/type.go View 1 7 chunks +15 lines, -6 lines 0 comments Download
M src/pkg/reflect/value.go View 1 2 3 16 chunks +49 lines, -25 lines 0 comments Download
M src/pkg/runtime/alg.go View 1 4 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/runtime/heapdump.c View 1 2 3 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/runtime/iface.go View 1 6 chunks +6 lines, -6 lines 0 comments Download
M src/pkg/runtime/malloc.c View 1 2 chunks +4 lines, -4 lines 0 comments Download
M src/pkg/runtime/malloc.go View 1 1 chunk +0 lines, -9 lines 0 comments Download
M src/pkg/runtime/mgc0.c View 1 2 3 3 chunks +3 lines, -3 lines 0 comments Download
M src/pkg/runtime/stack.c View 1 2 chunks +2 lines, -2 lines 0 comments Download
M src/pkg/runtime/typekind.h View 1 1 chunk +2 lines, -1 line 0 comments Download
A src/pkg/runtime/typekind.go View 1 1 chunk +44 lines, -0 lines 0 comments Download
M test/live.go View 1 11 chunks +27 lines, -24 lines 0 comments Download

Messages

Total messages: 8
rsc
Hello golang-codereviews@googlegroups.com (cc: iant, r), I'd like you to review this change to https://code.google.com/p/go/
3 years, 1 month ago (2014-08-13 15:08:45 UTC) #1
iant
LGTM https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go File src/cmd/pack/pack_test.go (right): https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go#newcode232 src/cmd/pack/pack_test.go:232: //defer os.RemoveAll(dir) This change is unrelated to this ...
3 years, 1 month ago (2014-08-13 15:54:16 UTC) #2
iant
If you turn on IfacePointerOnly then I think the scalar field of reflect.Value is no ...
3 years, 1 month ago (2014-08-13 17:47:58 UTC) #3
rsc
https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go File src/cmd/pack/pack_test.go (right): https://codereview.appspot.com/129090043/diff/20001/src/cmd/pack/pack_test.go#newcode232 src/cmd/pack/pack_test.go:232: //defer os.RemoveAll(dir) On 2014/08/13 15:54:15, iant wrote: > This ...
3 years, 1 month ago (2014-08-13 18:34:07 UTC) #4
rsc
On 2014/08/13 17:47:58, iant wrote: > If you turn on IfacePointerOnly then I think the ...
3 years, 1 month ago (2014-08-19 01:12:43 UTC) #5
rsc
https://codereview.appspot.com/129090043/diff/20001/src/pkg/reflect/value.go File src/pkg/reflect/value.go (right): https://codereview.appspot.com/129090043/diff/20001/src/pkg/reflect/value.go#newcode614 src/pkg/reflect/value.go:614: // value does not fit in word. On 2014/08/13 ...
3 years, 1 month ago (2014-08-19 01:12:50 UTC) #6
rsc
*** Submitted as https://code.google.com/p/go/source/detail?r=d9b3cbdfc09f *** cmd/gc, runtime: refactor interface inlining decision into compiler We need ...
3 years, 1 month ago (2014-08-19 01:13:16 UTC) #7
gobot
3 years, 1 month ago (2014-08-19 02:27:52 UTC) #8
Message was sent while issue was closed.
This CL appears to have broken the windows-386 builder.
See http://build.golang.org/log/bce4cb056acac994864e8b6c40b31574945e2168
Sign in to reply to this message.

Powered by Google App Engine
RSS Feeds Recent Issues | This issue
This is Rietveld 80a51fa-tainted