FAQ
Reviewers: golang-dev1,

Message:
Hello golang-dev@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/


Description:
cmd/gc: don't generate garbage in m[string(byteSlice)]

Fixes issue 3512

Please review this at https://codereview.appspot.com/8090046/

Affected files:
M src/cmd/gc/builtin.c
M src/cmd/gc/go.h
M src/cmd/gc/runtime.go
M src/cmd/gc/walk.c
M src/pkg/runtime/map_test.go
M src/pkg/runtime/string.goc


Index: src/cmd/gc/builtin.c
===================================================================
--- a/src/cmd/gc/builtin.c
+++ b/src/cmd/gc/builtin.c
@@ -31,6 +31,7 @@
"func @\"\".eqstring (? string, ? string) (? bool)\n"
"func @\"\".intstring (? int64) (? string)\n"
"func @\"\".slicebytetostring (? []byte) (? string)\n"
+ "func @\"\".slicebytetofakestring (? []byte) (? string)\n"
"func @\"\".slicerunetostring (? []rune) (? string)\n"
"func @\"\".stringtoslicebyte (? string) (? []byte)\n"
"func @\"\".stringtoslicerune (? string) (? []rune)\n"
Index: src/cmd/gc/go.h
===================================================================
--- a/src/cmd/gc/go.h
+++ b/src/cmd/gc/go.h
@@ -269,6 +269,7 @@
uchar dupok; // duplicate definitions ok (for func)
schar likely; // likeliness of if statement
uchar hasbreak; // has break statement
+ uchar nocopy; // avoid OARRAYBYTESTR copy when String doesn't escape

// most nodes
Type* type;
Index: src/cmd/gc/runtime.go
===================================================================
--- a/src/cmd/gc/runtime.go
+++ b/src/cmd/gc/runtime.go
@@ -48,6 +48,7 @@
func eqstring(string, string) bool
func intstring(int64) string
func slicebytetostring([]byte) string
+func slicebytetofakestring([]byte) string
func slicerunetostring([]rune) string
func stringtoslicebyte(string) []byte
func stringtoslicerune(string) []rune
Index: src/cmd/gc/walk.c
===================================================================
--- a/src/cmd/gc/walk.c
+++ b/src/cmd/gc/walk.c
@@ -659,6 +659,9 @@
break;
case TSTRING:
p = "mapaccess2_faststr";
+ if(r->right->op == OARRAYBYTESTR) {
+ r->right->nocopy = 1;
+ }
break;
}
}
@@ -1066,6 +1069,10 @@
break;
case TSTRING:
p = "mapaccess1_faststr";
+ r = n->right;
+ if(r->op == OARRAYBYTESTR) {
+ r->nocopy = 1;
+ }
break;
}
}
@@ -1268,7 +1275,11 @@

case OARRAYBYTESTR:
// slicebytetostring([]byte) string;
- n = mkcall("slicebytetostring", n->type, init, n->left);
+ if(n->nocopy)
+ fn = syslook("slicebytetofakestring", 1);
+ else
+ fn = syslook("slicebytetostring", 1);
+ n = mkcall1(fn, n->type, init, n->left);
goto ret;

case OARRAYRUNESTR:
Index: src/pkg/runtime/map_test.go
===================================================================
--- a/src/pkg/runtime/map_test.go
+++ b/src/pkg/runtime/map_test.go
@@ -300,6 +300,37 @@
}
}

+func TestMapStringKeyFromByteMallocs(t *testing.T) {
+ if runtime.Compiler != "gc" {
+ t.Skipf("untested on anything but gc")
+ }
+ const key, value = "some key", 42
+ m := map[string]int{
+ key: value,
+ }
+
+ b := []byte(key)
+ if n := testing.AllocsPerRun(20, func() { _ = string(b) }); n < 1 {
+ t.Errorf("expected mallocs for string([]byte); got %v", n)
+ }
+
+ if n := testing.AllocsPerRun(100, func() {
+ if m[string(b)] != value {
+ panic("wrong lookup")
+ }
+ }); n > 0 {
+ t.Errorf("lookup1 mallocs = %v; want 0", n)
+ }
+
+ if n := testing.AllocsPerRun(100, func() {
+ if v, ok := m[string(b)]; v != value || !ok {
+ panic("wrong lookup")
+ }
+ }); n > 0 {
+ t.Errorf("lookup2 mallocs = %v; want 0", n)
+ }
+}
+
type empty struct {
}

Index: src/pkg/runtime/string.goc
===================================================================
--- a/src/pkg/runtime/string.goc
+++ b/src/pkg/runtime/string.goc
@@ -282,6 +282,18 @@
runtime·memmove(s.str, b.array, s.len);
}

+// Returns a fake String sharing memory with b that must not
+// escape. Only used for hash lookup functions.
+func slicebytetofakestring(b Slice) (s String) {
+ void *pc;
+ if(raceenabled) {
+ pc = runtime·getcallerpc(&b);
+ runtime·racereadrangepc(b.array, b.len, 1, pc,
runtime·slicebytetofakestring);
+ }
+ s.str = b.array;
+ s.len = b.len;
+}
+
func stringtoslicebyte(s String) (b Slice) {
b.array = runtime·mallocgc(s.len, FlagNoPointers, 1, 0);
b.len = s.len;


--

---
You received this message because you are subscribed to the Google Groups "golang-dev" group.
To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
For more options, visit https://groups.google.com/groups/opt_out.

Search Discussions

  • Remyoudompheng at Apr 1, 2013 at 10:34 pm
    I wouldn't have done like that. I think I would have introduced a new
    node type OSTRARRAYBYTENOP that does the no-op conversion, instrumented
    it in racewalk, and implemented it in gen.c like cgen_slice.

    But it's a more intrusive way of doing.

    A test should be added in runtime/race to check that the race is
    correctly detected.

    Is it totally guaranteed that the hashmap will not keep the key inside?
    The lazy-growth-on-map-access frightens me.

    https://codereview.appspot.com/8090046/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Brad Fitzpatrick at Apr 1, 2013 at 10:44 pm

    On Mon, Apr 1, 2013 at 3:34 PM, wrote:

    I wouldn't have done like that. I think I would have introduced a new
    node type OSTRARRAYBYTENOP that does the no-op conversion, instrumented
    it in racewalk, and implemented it in gen.c like cgen_slice.

    But it's a more intrusive way of doing.
    I'm new here. :-)

    I considered that, but didn't know conventions, and like you said--- it
    seemed more intrusive.

    A test should be added in runtime/race to check that the race is
    correctly detected.
    Will do.

    Is it totally guaranteed that the hashmap will not keep the key inside?
    >

    Yes.

    The lazy-growth-on-map-access frightens me.
    That's a bug and is being fixed. golang.org/issue/5179

    The only thing this impacts is per-map hash caching (golang.org/issue/5147)
    which retains the key per-M between lookups to avoid future hashes. But
    after implementing that, the numbers suggest we'll need compiler support to
    give hints about when it's beneficial anyway, so the compiler can not pass
    the "it is beneficial" hint to the lookup function here when nocopy is in
    use.

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.
  • Bradfitz at Apr 1, 2013 at 11:01 pm
    Hello golang-dev@googlegroups.com, remyoudompheng@gmail.com (cc:
    golang-dev@googlegroups.com),

    Please take another look.


    https://codereview.appspot.com/8090046/

    --

    ---
    You received this message because you are subscribed to the Google Groups "golang-dev" group.
    To unsubscribe from this group and stop receiving emails from it, send an email to golang-dev+unsubscribe@googlegroups.com.
    For more options, visit https://groups.google.com/groups/opt_out.

Related Discussions

Discussion Navigation
viewthread | post
Discussion Overview
groupgolang-dev @
categoriesgo
postedApr 1, '13 at 10:15p
activeApr 1, '13 at 11:01p
posts4
users2
websitegolang.org

2 users in discussion

Bradfitz: 3 posts Remyoudompheng: 1 post

People

Translate

site design / logo © 2022 Grokbase