Repository: google/starlark-go (go)

Issue:
The int optimization poses problems for users running with ulimit
Reference: https://github.com/go-delve/delve/issues/2907

The code in int_posix64.go assumes that the program has the ability to make a 4G allocation, this is not necessarily the case if the system is using ulimit to limit the amount of memory that processes can allocate. Given that this is an optimization the interpreter should be able to continue functioning even if the allocation fails (possibly attempting a smaller allocation first?)

Pull request:
starlark: fall back to slow Ints if mmap fails
The Int optimization uses a 4GB virtual address allocation to represent
32-bit values as addresses to avoid allocation. However, some environments
have limited address space.

This change permits the mmap to fail, and in that case it prints a warning
and falls back to always allocating a big.Int, even for small numbers.
Each access to an Int must check the smallints global to see whether the
optimization is active.  The extra dynamic check doesn't cost much:

```
$ go test -run=nope -bench=BenchmarkStarlark -count=3 ./starlark    # > before, after
$ go install golang.org/x/perf/cmd/benchstat@latest
$ ~/go/bin/benchstat  before after
name                                  old time/op  new time/op   delta
Starlark/bench_bigint-16               206µs ± 0%    212µs ± 0%   ~     (p=0.100 n=3+3)
Starlark/bench_builtin_method-16       283µs ± 1%    294µs ± 9%   ~     (p=1.000 n=3+3)
Starlark/bench_calling-16              280µs ± 1%    290µs ± 1%   ~     (p=0.100 n=3+3)
Starlark/bench_gauss-16               9.30ms ± 5%   9.55ms ± 2%   ~     (p=0.700 n=3+3)
Starlark/bench_int-16                 53.7µs ± 1%   59.6µs ± 1%   ~     (p=0.100 n=3+3)
Starlark/bench_mix-16                 99.3µs ± 1%  106.6µs ±10%   ~     (p=0.100 n=3+3)
Starlark/bench_range_construction-16   238ns ± 1%    244ns ± 2%   ~     (p=0.100 n=3+3)
Starlark/bench_range_iteration-16     5.44µs ± 2%   5.67µs ± 1%   ~     (p=0.100 n=3+3)
```

Also, a linux-only test.

Also, simplify the build tags now that the 64-bit POSIX exceptions (iOS, openbsd)
are handled dynamically. As a side effect, M1-based Macs should get the optimization
for the first time. (Requires updating sys module.)


Fixes #394



Patch:
diff --git a/go.mod b/go.mod
index d14060e..ae5f371 100644
--- a/go.mod
+++ b/go.mod
@@ -7,7 +7,7 @@ require (
 	github.com/chzyer/readline v0.0.0-20180603132655-2972be24d48e
 	github.com/chzyer/test v0.0.0-20180213035817-a1ea475d72b1 // indirect
 	github.com/google/go-cmp v0.5.1 // indirect
-	golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f
+	golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab
 	golang.org/x/xerrors v0.0.0-20200804184101-5ec99f83aff1 // indirect
 	google.golang.org/protobuf v1.25.0
 )
diff --git a/go.sum b/go.sum
index 90a8048..62e9fda 100644
--- a/go.sum
+++ b/go.sum
@@ -43,8 +43,8 @@ golang.org/x/sync v0.0.0-20181108010431-42b317875d0f/go.mod h1:RxMgew5VJxzue5/jJ
 golang.org/x/sync v0.0.0-20190423024810-112230192c58/go.mod h1:RxMgew5VJxzue5/jJTE5uejpjVlOe/izrB70Jof72aM=
 golang.org/x/sys v0.0.0-20180830151530-49385e6e1522/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
 golang.org/x/sys v0.0.0-20190215142949-d0b11bdaac8a/go.mod h1:STP8DvDyc/dI5b8T5hshtkjS+E42TnysNCUPdjciGhY=
-golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f h1:+Nyd8tzPX9R7BWHguqsrbFdRx3WQ/1ib8I44HXV5yTA=
-golang.org/x/sys v0.0.0-20200930185726-fdedc70b468f/go.mod h1:h1NjWce9XRLGQEsW7wpKNCjG9DtNlClVuFLEZdDNbEs=
+golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab h1:2QkjZIsXupsJbJIdSjjUOgWK3aEtzyuh2mPt3l/CkeU=
+golang.org/x/sys v0.0.0-20220811171246-fbc7d0a398ab/go.mod h1:oPkhp1MJrh7nUepCBck5+mAzfO9JrbApNNgaTdGDITg=
 golang.org/x/text v0.3.0/go.mod h1:NqM8EUOU14njkJ3fqMW+pc6Ldnwhi/IjpwHt7yyuwOQ=
 golang.org/x/tools v0.0.0-20190114222345-bf090417da8b/go.mod h1:n7NCudcB/nEzxVGmLbDWY5pfWTLqBcC2KZ6jyYvM4mQ=
 golang.org/x/tools v0.0.0-20190226205152-f727befe758c/go.mod h1:9Yl7xja0Znq3iFh3HoIrodX9oNMXvdceNzlUR8zjMvY=
diff --git a/starlark/int.go b/starlark/int.go
index e6dee14..398811b 100644
--- a/starlark/int.go
+++ b/starlark/int.go
@@ -46,13 +46,18 @@ func MakeUint64(x uint64) Int {
 // MakeBigInt returns a Starlark int for the specified big.Int.
 // The new Int value will contain a copy of x. The caller is safe to modify x.
 func MakeBigInt(x *big.Int) Int {
-	if n := x.BitLen(); n < 32 || n == 32 && x.Int64() == math.MinInt32 {
+	if isSmall(x) {
 		return makeSmallInt(x.Int64())
 	}
 	z := new(big.Int).Set(x)
 	return makeBigInt(z)
 }
 
+func isSmall(x *big.Int) bool {
+	n := x.BitLen()
+	return n < 32 || n == 32 && x.Int64() == math.MinInt32
+}
+
 var (
 	zero, one = makeSmallInt(0), makeSmallInt(1)
 	oneBig    = big.NewInt(1)
diff --git a/starlark/int_generic.go b/starlark/int_generic.go
index 8363ae0..f2adf45 100644
--- a/starlark/int_generic.go
+++ b/starlark/int_generic.go
@@ -1,4 +1,5 @@
-//+build !linux,!darwin,!dragonfly,!freebsd,!netbsd,!solaris darwin,arm64 !amd64,!arm64,!mips64x,!ppc64x,!loong64
+//go:build (!linux && !darwin && !dragonfly && !freebsd && !netbsd && !solaris) || (!amd64 && !arm64 && !mips64x && !ppc64x && !loong64)
+// +build !linux,!darwin,!dragonfly,!freebsd,!netbsd,!solaris !amd64,!arm64,!mips64x,!ppc64x,!loong64
 
 package starlark
 
diff --git a/starlark/int_posix64.go b/starlark/int_posix64.go
index 71b1e75..2ab0bed 100644
--- a/starlark/int_posix64.go
+++ b/starlark/int_posix64.go
@@ -1,5 +1,6 @@
-//+build linux darwin dragonfly freebsd netbsd solaris
-//+build amd64 arm64,!darwin mips64x ppc64x loong64
+//go:build (linux || darwin || dragonfly || freebsd || netbsd || solaris) && (amd64 || arm64 || mips64x || ppc64x || loong64)
+// +build linux darwin dragonfly freebsd netbsd solaris
+// +build amd64 arm64 mips64x ppc64x loong64
 
 package starlark
 
@@ -10,16 +11,12 @@ package starlark
 // values be represented as an unsafe.Pointer, so that Int-to-Value
 // interface conversion need not allocate.
 
-// Although iOS (arm64,darwin) claims to be a POSIX-compliant,
-// it limits each process to about 700MB of virtual address space,
-// which defeats the optimization.
-//
-// TODO(golang.org/issue/38485): darwin,arm64 may refer to macOS in the future.
-// Update this when there are distinct GOOS values for macOS, iOS, and other Apple
-// operating systems on arm64.
-//
-// This optimization is disabled on OpenBSD, because its default
-// ulimit for virtual memory is a measly GB or so.
+// Although iOS (which, like macOS, appears as darwin/arm64) is
+// POSIX-compliant, it limits each process to about 700MB of virtual
+// address space, which defeats the optimization.  Similarly,
+// OpenBSD's default ulimit for virtual memory is a measly GB or so.
+// On both those platforms the attempted optimization will fail and
+// fall back to the slow implementation.
 
 // An alternative approach to this optimization would be to embed the
 // int32 values in pointers using odd values, which can be distinguished
@@ -41,15 +38,24 @@ import (
 // so that Int-to-Value conversions need not allocate.
 //
 // The pointer is either a *big.Int, if the value is big, or a pointer into a
-// reserved portion of the address space (smallints), if the value is small.
+// reserved portion of the address space (smallints), if the value is small
+// and the address space allocation succeeded.
 //
 // See int_generic.go for the basic representation concepts.
 type intImpl unsafe.Pointer
 
 // get returns the (small, big) arms of the union.
 func (i Int) get() (int64, *big.Int) {
-	ptr := uintptr(i.impl)
-	if ptr >= smallints && ptr < smallints+1<<32 {
+	if smallints == 0 {
+		// optimization disabled
+		if x := (*big.Int)(i.impl); isSmall(x) {
+			return x.Int64(), nil
+		} else {
+			return 0, x
+		}
+	}
+
+	if ptr := uintptr(i.impl); ptr >= smallints && ptr < smallints+1<<32 {
 		return math.MinInt32 + int64(ptr-smallints), nil
 	}
 	return 0, (*big.Int)(i.impl)
@@ -57,6 +63,11 @@ func (i Int) get() (int64, *big.Int) {
 
 // Precondition: math.MinInt32 <= x && x <= math.MaxInt32
 func makeSmallInt(x int64) Int {
+	if smallints == 0 {
+		// optimization disabled
+		return Int{intImpl(big.NewInt(x))}
+	}
+
 	return Int{intImpl(uintptr(x-math.MinInt32) + smallints)}
 }
 
@@ -66,12 +77,15 @@ func makeBigInt(x *big.Int) Int { return Int{intImpl(x)} }
 // smallints is the base address of a 2^32 byte memory region.
 // Pointers to addresses in this region represent int32 values.
 // We assume smallints is not at the very top of the address space.
+//
+// Zero means the optimization is disabled and all Ints allocate a big.Int.
 var smallints = reserveAddresses(1 << 32)
 
 func reserveAddresses(len int) uintptr {
 	b, err := unix.Mmap(-1, 0, len, unix.PROT_READ, unix.MAP_PRIVATE|unix.MAP_ANON)
 	if err != nil {
-		log.Fatalf("mmap: %v", err)
+		log.Printf("Starlark failed to allocate 4GB address space: %v. Integer performance may suffer.", err)
+		return 0 // optimization disabled
 	}
 	return uintptr(unsafe.Pointer(&b[0]))
 }


Test patch:
diff --git a/.github/workflows/tests.yml b/.github/workflows/tests.yml
index 4944036..893cc11 100644
--- a/.github/workflows/tests.yml
+++ b/.github/workflows/tests.yml
@@ -5,7 +5,7 @@ jobs:
     strategy:
       matrix:
         os: [ubuntu-latest, macos-latest, windows-latest]
-        go-version: [1.16.x, 1.17.x]
+        go-version: [1.18.x, 1.19.x]
     runs-on: ${{ matrix.os }}
     steps:
       - name: Install Go
diff --git a/starlark/bench_test.go b/starlark/bench_test.go
index 7cfefe0..e62cf59 100644
--- a/starlark/bench_test.go
+++ b/starlark/bench_test.go
@@ -16,7 +16,7 @@ import (
 	"go.starlark.net/starlarktest"
 )
 
-func Benchmark(b *testing.B) {
+func BenchmarkStarlark(b *testing.B) {
 	defer setOptions("")
 
 	testdata := starlarktest.DataFile("starlark", ".")
diff --git a/starlark/int_test.go b/starlark/int_test.go
index ad1bf92..8c20a31 100644
--- a/starlark/int_test.go
+++ b/starlark/int_test.go
@@ -5,9 +5,15 @@
 package starlark
 
 import (
+	"flag"
 	"fmt"
+	"log"
 	"math"
 	"math/big"
+	"os"
+	"os/exec"
+	"runtime"
+	"strings"
 	"testing"
 )
 
@@ -100,3 +106,50 @@ func TestImmutabilityBigInt(t *testing.T) {
 		}
 	}
 }
+
+// TestIntFallback creates a small Int value in a child process with
+// limited address space to ensure that it still works, but prints a warning.
+func TestIntFallback(t *testing.T) {
+	if runtime.GOOS != "linux" {
+		t.Skipf("test disabled on this platform (requires ulimit -v)")
+	}
+	exe, err := os.Executable()
+	if err != nil {
+		t.Fatalf("can't find file name of executable: %v", err)
+	}
+	// ulimit -v limits the address space in KB. Not portable.
+	// 1GB is enough for the Go runtime but not for the optimization.
+	cmd := exec.Command("/bin/sh", "-c", fmt.Sprintf("ulimit -v 1000000 && %q --entry=intfallback", exe))
+	out, err := cmd.CombinedOutput()
+	if err != nil {
+		t.Fatalf("intfallback subcommand failed: %v\n%s", err, out)
+	}
+
+	// Check the warning was printed.
+	if !strings.Contains(string(out), "Integer performance may suffer") {
+		t.Errorf("expected warning was not printed. Output=<<%s>>", out)
+	}
+}
+
+// intfallback is called in a child process with limited address space.
+func intfallback() {
+	const want = 123
+	if got, _ := MakeBigInt(big.NewInt(want)).Int64(); got != want {
+		log.Fatalf("intfallback: got %d, want %d", got, want)
+	}
+}
+
+// The --entry flag invokes an alternate entry point, for use in subprocess tests.
+func TestMain(m *testing.M) {
+	var entry string
+	flag.StringVar(&entry, "entry", "", "child process entry-point")
+	flag.Parse()
+	switch entry {
+	case "":
+		os.Exit(m.Run()) // normal case
+	case "intfallback":
+		intfallback()
+	default:
+		log.Fatalf("unknown entry point: %s", entry)
+	}
+}