From 788e038e5d5fcdc1cc44ec9af1885db55a19977c Mon Sep 17 00:00:00 2001
From: =?utf8?q?Daniel=20Mart=C3=AD?= <mvdan@mvdan.cc>
Date: Sat, 9 Mar 2019 18:09:10 +0000
Subject: [PATCH] reflect: make all flag.mustBe* methods inlinable
MIME-Version: 1.0
Content-Type: text/plain; charset=utf8
Content-Transfer-Encoding: 8bit

mustBe was barely over budget, so manually inlining the first flag.kind
call is enough. Add a TODO to reverse that in the future, once the
compiler gets better.

mustBeExported and mustBeAssignable were over budget by a larger amount,
so add slow path functions instead. This is the same strategy used in
the sync package for common methods like Once.Do, for example.

Lots of exported reflect.Value methods call these assert-like unexported
methods, so avoiding the function call overhead in the common case does
shave off a percent from most exported APIs.

Finally, add the methods to TestIntendedInlining.

While at it, replace a couple of uses of the 0 Kind with its descriptive
name, Invalid.

name     old time/op    new time/op    delta
Call-8     68.0ns ± 1%    66.8ns ± 1%  -1.81%  (p=0.000 n=10+9)
PtrTo-8    8.00ns ± 2%    7.83ns ± 0%  -2.19%  (p=0.000 n=10+9)

Updates #7818.

Change-Id: Ic1603b640519393f6b50dd91ec3767753eb9e761
Reviewed-on: https://go-review.googlesource.com/c/go/+/166462
Run-TryBot: Daniel Martí <mvdan@mvdan.cc>
TryBot-Result: Gobot Gobot <gobot@golang.org>
Reviewed-by: Brad Fitzpatrick <bradfitz@golang.org>
---
 src/cmd/compile/internal/gc/inl_test.go |  9 +++------
 src/reflect/value.go                    | 19 ++++++++++++++++---
 2 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/src/cmd/compile/internal/gc/inl_test.go b/src/cmd/compile/internal/gc/inl_test.go
index 1ad6ca3421..0dfd252372 100644
--- a/src/cmd/compile/internal/gc/inl_test.go
+++ b/src/cmd/compile/internal/gc/inl_test.go
@@ -133,14 +133,11 @@ func TestIntendedInlining(t *testing.T) {
 			"Value.pointer",
 			"add",
 			"align",
+			"flag.mustBe",
+			"flag.mustBeAssignable",
+			"flag.mustBeExported",
 			"flag.kind",
 			"flag.ro",
-
-			// TODO: these use panic, which gets their budgets
-			// slightly over the limit
-			// "flag.mustBe",
-			// "flag.mustBeAssignable",
-			// "flag.mustBeExported",
 		},
 		"regexp": {
 			"(*bitState).push",
diff --git a/src/reflect/value.go b/src/reflect/value.go
index 372b7a6dc8..5951b18b8c 100644
--- a/src/reflect/value.go
+++ b/src/reflect/value.go
@@ -203,7 +203,8 @@ type nonEmptyInterface struct {
 // v.flag.mustBe(Bool), which will only bother to copy the
 // single important word for the receiver.
 func (f flag) mustBe(expected Kind) {
-	if f.kind() != expected {
+	// TODO(mvdan): use f.kind() again once mid-stack inlining gets better
+	if Kind(f&flagKindMask) != expected {
 		panic(&ValueError{methodName(), f.kind()})
 	}
 }
@@ -211,8 +212,14 @@ func (f flag) mustBe(expected Kind) {
 // mustBeExported panics if f records that the value was obtained using
 // an unexported field.
 func (f flag) mustBeExported() {
+	if f == 0 || f&flagRO != 0 {
+		f.mustBeExportedSlow()
+	}
+}
+
+func (f flag) mustBeExportedSlow() {
 	if f == 0 {
-		panic(&ValueError{methodName(), 0})
+		panic(&ValueError{methodName(), Invalid})
 	}
 	if f&flagRO != 0 {
 		panic("reflect: " + methodName() + " using value obtained using unexported field")
@@ -223,6 +230,12 @@ func (f flag) mustBeExported() {
 // which is to say that either it was obtained using an unexported field
 // or it is not addressable.
 func (f flag) mustBeAssignable() {
+	if f&flagRO != 0 || f&flagAddr == 0 {
+		f.mustBeAssignableSlow()
+	}
+}
+
+func (f flag) mustBeAssignableSlow() {
 	if f == 0 {
 		panic(&ValueError{methodName(), Invalid})
 	}
@@ -981,7 +994,7 @@ func (v Value) Interface() (i interface{}) {
 
 func valueInterface(v Value, safe bool) interface{} {
 	if v.flag == 0 {
-		panic(&ValueError{"reflect.Value.Interface", 0})
+		panic(&ValueError{"reflect.Value.Interface", Invalid})
 	}
 	if safe && v.flag&flagRO != 0 {
 		// Do not allow access to unexported values via Interface,
-- 
2.50.0