Permalink
Browse files

cmd/compile: avoid mapaccess at m[k]=append(m[k]..

Currently rvalue m[k] is transformed during walk into:

        tmp1 := *mapaccess(m, k)
        tmp2 := append(tmp1, ...)
        *mapassign(m, k) = tmp2

However, this is suboptimal, as we could instead produce just:
        tmp := mapassign(m, k)
        *tmp := append(*tmp, ...)

Optimization is possible only if during Order it may tell that m[k] is
exactly the same at left and right part of assignment. It doesn't work:
1) m[f(k)] = append(m[f(k)], ...)
2) sink, m[k] = sink, append(m[k]...)
3) m[k] = append(..., m[k],...)

Benchmark:
name                           old time/op    new time/op    delta
MapAppendAssign/Int32/256-8      33.5ns ± 3%    22.4ns ±10%  -33.24%  (p=0.000 n=16+18)
MapAppendAssign/Int32/65536-8    68.2ns ± 6%    48.5ns ±29%  -28.90%  (p=0.000 n=20+20)
MapAppendAssign/Int64/256-8      34.3ns ± 4%    23.3ns ± 5%  -32.23%  (p=0.000 n=17+18)
MapAppendAssign/Int64/65536-8    65.9ns ± 7%    61.2ns ±19%   -7.06%  (p=0.002 n=18+20)
MapAppendAssign/Str/256-8         116ns ±12%      79ns ±16%  -31.70%  (p=0.000 n=20+19)
MapAppendAssign/Str/65536-8       134ns ±15%     111ns ±45%  -16.95%  (p=0.000 n=19+20)

name                           old alloc/op   new alloc/op   delta
MapAppendAssign/Int32/256-8       47.0B ± 0%     46.0B ± 0%   -2.13%  (p=0.000 n=19+18)
MapAppendAssign/Int32/65536-8     27.0B ± 0%     20.7B ±30%  -23.33%  (p=0.000 n=20+20)
MapAppendAssign/Int64/256-8       47.0B ± 0%     46.0B ± 0%   -2.13%  (p=0.000 n=20+17)
MapAppendAssign/Int64/65536-8     27.0B ± 0%     27.0B ± 0%     ~     (all equal)
MapAppendAssign/Str/256-8         94.0B ± 0%     78.0B ± 0%  -17.02%  (p=0.000 n=20+16)
MapAppendAssign/Str/65536-8       54.0B ± 0%     54.0B ± 0%     ~     (all equal)

Fixes #24364
Updates #5147

Change-Id: Id257d052b75b9a445b4885dc571bf06ce6f6b409
Reviewed-on: https://go-review.googlesource.com/100838
Reviewed-by: Matthew Dempsky <mdempsky@google.com>
Run-TryBot: Matthew Dempsky <mdempsky@google.com>
TryBot-Result: Gobot Gobot <gobot@golang.org>
  • Loading branch information...
vkuzmin-uber authored and mdempsky committed Mar 14, 2018
1 parent e22d241 commit c12b185a6ed143e7b397bd58489866505756be0e
@@ -438,7 +438,14 @@ func (o *Order) mapAssign(n *Node) {
if n.Left.Op == OINDEXMAP {
// Make sure we evaluate the RHS before starting the map insert.
// We need to make sure the RHS won't panic. See issue 22881.
n.Right = o.cheapExpr(n.Right)
if n.Right.Op == OAPPEND {
s := n.Right.List.Slice()[1:]
for i, n := range s {
s[i] = o.cheapExpr(n)
}
} else {
n.Right = o.cheapExpr(n.Right)
}
}
o.out = append(o.out, n)
@@ -3231,8 +3231,21 @@ func checkassignlist(stmt *Node, l Nodes) {
}
}
// Check whether l and r are the same side effect-free expression,
// so that it is safe to reuse one instead of computing both.
// samesafeexpr checks whether it is safe to reuse one of l and r
// instead of computing both. samesafeexpr assumes that l and r are
// used in the same statement or expression. In order for it to be
// safe to reuse l or r, they must:
// * be the same expression
// * not have side-effects (no function calls, no channel ops);
// however, panics are ok
// * not cause inappropriate aliasing; e.g. two string to []byte
// conversions, must result in two distinct slices
//
// The handling of OINDEXMAP is subtle. OINDEXMAP can occur both
// as an lvalue (map assignment) and an rvalue (map access). This is
// currently OK, since the only place samesafeexpr gets used on an
// lvalue expression is for OSLICE and OAPPEND optimizations, and it
// is correct in those settings.
func samesafeexpr(l *Node, r *Node) bool {
if l.Op != r.Op || !eqtype(l.Type, r.Type) {
return false
@@ -3253,7 +3266,7 @@ func samesafeexpr(l *Node, r *Node) bool {
// Allow only numeric-ish types. This is a bit conservative.
return issimple[l.Type.Etype] && samesafeexpr(l.Left, r.Left)
case OINDEX:
case OINDEX, OINDEXMAP:
return samesafeexpr(l.Left, r.Left) && samesafeexpr(l.Right, r.Right)
case OLITERAL:
@@ -670,9 +670,20 @@ opswitch:
case OAS, OASOP:
init.AppendNodes(&n.Ninit)
// Recognize m[k] = append(m[k], ...) so we can reuse
// the mapassign call.
mapAppend := n.Left.Op == OINDEXMAP && n.Right.Op == OAPPEND
if mapAppend && !samesafeexpr(n.Left, n.Right.List.First()) {
Fatalf("not same expressions: %v != %v", n.Left, n.Right.List.First())
}
n.Left = walkexpr(n.Left, init)
n.Left = safeexpr(n.Left, init)
if mapAppend {
n.Right.List.SetFirst(n.Left)
}
if n.Op == OASOP {
// Rewrite x op= y into x = x op y.
n.Right = nod(n.SubOp(), n.Left, n.Right)
@@ -114,6 +114,24 @@ func TestMapOperatorAssignment(t *testing.T) {
}
}
var sinkAppend bool
func TestMapAppendAssignment(t *testing.T) {
m := make(map[int][]int, 0)
m[0] = nil
m[0] = append(m[0], 12345)
m[0] = append(m[0], 67890)
sinkAppend, m[0] = !sinkAppend, append(m[0], 123, 456)
a := []int{7, 8, 9, 0}
m[0] = append(m[0], a...)
want := []int{12345, 67890, 123, 456, 7, 8, 9, 0}
if got := m[0]; !reflect.DeepEqual(got, want) {
t.Errorf("got %v, want %v", got, want)
}
}
// Maps aren't actually copied on assignment.
func TestAlias(t *testing.T) {
m := make(map[int]int, 0)
@@ -839,6 +857,16 @@ func benchmarkMapOperatorAssignInt32(b *testing.B, n int) {
}
}
func benchmarkMapAppendAssignInt32(b *testing.B, n int) {
a := make(map[int32][]int)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
key := int32(i & (n - 1))
a[key] = append(a[key], i)
}
}
func benchmarkMapDeleteInt32(b *testing.B, n int) {
a := make(map[int32]int, n)
b.ResetTimer()
@@ -868,6 +896,16 @@ func benchmarkMapOperatorAssignInt64(b *testing.B, n int) {
}
}
func benchmarkMapAppendAssignInt64(b *testing.B, n int) {
a := make(map[int64][]int)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
key := int64(i & (n - 1))
a[key] = append(a[key], i)
}
}
func benchmarkMapDeleteInt64(b *testing.B, n int) {
a := make(map[int64]int, n)
b.ResetTimer()
@@ -908,6 +946,20 @@ func benchmarkMapOperatorAssignStr(b *testing.B, n int) {
}
}
func benchmarkMapAppendAssignStr(b *testing.B, n int) {
k := make([]string, n)
for i := 0; i < len(k); i++ {
k[i] = strconv.Itoa(i)
}
a := make(map[string][]string)
b.ReportAllocs()
b.ResetTimer()
for i := 0; i < b.N; i++ {
key := k[i&(n-1)]
a[key] = append(a[key], key)
}
}
func benchmarkMapDeleteStr(b *testing.B, n int) {
i2s := make([]string, n)
for i := 0; i < n; i++ {
@@ -949,6 +1001,12 @@ func BenchmarkMapOperatorAssign(b *testing.B) {
b.Run("Str", runWith(benchmarkMapOperatorAssignStr, 1<<8, 1<<16))
}
func BenchmarkMapAppendAssign(b *testing.B) {
b.Run("Int32", runWith(benchmarkMapAppendAssignInt32, 1<<8, 1<<16))
b.Run("Int64", runWith(benchmarkMapAppendAssignInt64, 1<<8, 1<<16))
b.Run("Str", runWith(benchmarkMapAppendAssignStr, 1<<8, 1<<16))
}
func BenchmarkMapDelete(b *testing.B) {
b.Run("Int32", runWith(benchmarkMapDeleteInt32, 100, 1000, 10000))
b.Run("Int64", runWith(benchmarkMapDeleteInt64, 100, 1000, 10000))
Oops, something went wrong.

0 comments on commit c12b185

Please sign in to comment.