perf: environment scan tier, plus CI that can actually fail

.github/workflows/test.yml

@@ -3,32 +3,41 @@ name: Test
 on:
   push:
     branches: [ "1.0" ]
+  # Deliberately unfiltered by base branch, so a pull request is checked
+  # wherever it is targeted rather than only when it targets 1.0.
   pull_request:
-    branches: [ "1.0" ]
 
 jobs:
 
-  build:
-    name: Build
+  test:
+    name: Test
     runs-on: ubuntu-latest
     steps:
 
-    - name: Set up Go 1.x
-      uses: actions/setup-go@v2
+    - name: Check out code
+      uses: actions/checkout@v4
+
+    - name: Set up Go
+      uses: actions/setup-go@v5
       with:
-        go-version: ^1.17
-      id: go
+        # Track the version in go.mod rather than repeating it here.
+        go-version-file: go.mod
 
-    - name: Check out code into the Go module directory
-      uses: actions/checkout@v2
+    - name: Check formatting
+      run: make fmt
 
-    - name: Get dependencies
-      run: |
-        go get -v -t -d ./...
-        if [ -f Gopkg.toml ]; then
-            curl https://raw.githubusercontent.com/golang/dep/master/install.sh | sh
-            dep ensure
-        fi
+    - name: Vet
+      run: make vet
+
+    - name: Build
+      run: go build ./...
 
     - name: Test
       run: make test
+
+    # Benchmarks are not compiled by a plain `go test` run, and the Ghost
+    # programs inside them are only parsed once they execute, so a language
+    # change can leave the suite unable to parse while every test still passes.
+    # Running each benchmark a single iteration is enough to catch that.
+    - name: Benchmarks
+      run: make bench

Makefile

@@ -1,6 +1,11 @@
 GOPATH:=$(shell go env GOPATH)
 
-.PHONY: run build build-mac build-linux build-windows test clean
+# The test target pipes through sed for colour. A pipeline reports the exit
+# status of its last command, so without bash and pipefail a failing test run
+# is reported as success - which is what CI was doing.
+SHELL:=/bin/bash
+
+.PHONY: run build build-mac build-linux build-windows test bench check fmt vet clean
 
 run:
 	go run cmd/*.go
@@ -17,7 +22,21 @@ build-windows: clean
 	GOOS=windows go build -trimpath -o ./dist/windows/ghost.exe cmd/*.go
 
 test:
-	go test -v -race -timeout 5s ./... | sed ''/PASS/s//$$(printf "\033[32mPASS\033[0m")/'' | sed ''/FAIL/s//$$(printf "\033[31mFAIL\033[0m")/''
+	set -o pipefail; go test -v -race -timeout 120s ./... | sed ''/PASS/s//$$(printf "\033[32mPASS\033[0m")/'' | sed ''/FAIL/s//$$(printf "\033[31mFAIL\033[0m")/''
+
+# Benchmark bodies are not compiled into a plain `go test` run, and the Ghost
+# programs they hold are only parsed when they execute. Running each one once is
+# what catches a language change that leaves the suite unable to parse.
+bench:
+	go test -run '^$$' -bench=. -benchtime=1x ./...
+
+fmt:
+	@test -z "$$(gofmt -l . | tee /dev/stderr)" || (echo "run gofmt -w ." && exit 1)
+
+vet:
+	go vet ./...
+
+check: fmt vet test bench
 
 clean:
 	@rm -rf dist/mac

evaluator/benchmark_test.go

@@ -102,17 +102,48 @@ var benchmarks = []struct {
 		"ClassMethods",
 		`class Counter {
 			count = 0
-			function increment() {
+			increment() {
 				this.count = this.count + 1
 				return this.count
 			}
 		}
-		c = Counter.new()
+		c = new Counter()
 		for (i = 0; i < 10000; i = i + 1) {
 			c.increment()
 		}
 		c.count`,
 	},
+	{
+		// A game loop: many globals in one flat scope, read from a tight
+		// nested loop, with no function calls to push work into child scopes.
+		// This shape stresses lookup in a large scope rather than call setup,
+		// and the two pull environment storage in opposite directions.
+		"TileRender",
+		`layers = []
+		for (l = 0; l < 3; l = l + 1) {
+			rows = []
+			for (y = 0; y < 30; y = y + 1) {
+				row = []
+				for (x = 0; x < 40; x = x + 1) { row[x] = (x + y + l) % 8 }
+				rows[y] = row
+			}
+			layers[l] = rows
+		}
+		total = 0
+		for (frame = 0; frame < 10; frame = frame + 1) {
+			for (l = 0; l < 3; l = l + 1) {
+				rows = layers[l]
+				for (y = 0; y < 30; y = y + 1) {
+					row = rows[y]
+					for (x = 0; x < 40; x = x + 1) {
+						tile = row[x]
+						if (tile != 0) { total = total + tile * 16 + x * 16 + y * 16 }
+					}
+				}
+			}
+		}
+		total`,
+	},
 	{
 		"MapOperations",
 		`m = {"a": 1, "b": 2, "c": 3}

object/environment.go

@@ -5,23 +5,51 @@ import (
 	"os"
 )
 
-// inlineCapacity is how many variables an environment stores inline, in the
-// environment struct itself, before it falls back to a map.
+// Environments store their bindings in three tiers, in lookup order: a fixed
+// array inside the struct, a grown slice, and finally a map.
 //
-// Every function call builds a new environment, and almost all of them hold
-// only a handful of names: the parameters plus a few locals. A map is a poor
-// fit for that. Allocating one, and growing its buckets as names are set,
-// dominated the memory profile of call-heavy programs. Storing the first few
-// names inline means an environment costs a single allocation and lookups are
-// a short scan of adjacent memory rather than a hash. Environments that hold
-// more names than this - module and class scopes, mainly - spill the remainder
-// into the overflow map and keep their previous behavior.
-const inlineCapacity = 4
+// The tiers exist because the two shapes of scope in a Ghost program want
+// opposite things. A call frame holds a couple of parameters and is created
+// again on every call, so it wants to be small and to cost a single allocation;
+// the top level of a script holds every global and is read constantly from
+// inside hot loops, so it wants lookups to stay cheap well past a couple of
+// names. Sizing one fixed array for both made calls slower when it was large
+// and script globals slower when it was small.
+//
+// Scanning beats hashing at these sizes by more than it looks like it should.
+// A name being looked up and the name stored in the environment usually come
+// from the same identifier in the AST, so the string comparison settles on
+// equal pointers without examining any characters.
+//
+// Past scanLimit a scan really does stop paying for itself, so everything moves
+// into the map and the scan tiers are abandoned. Splitting names across a scan
+// tier and a map would be worse than either: every miss would pay a full scan
+// before the map lookup.
+const (
+	inlineCapacity = 4
+	scanLimit      = 16
+)
+
+// binding is one name/value pair in an environment's second storage tier.
+type binding struct {
+	name  string
+	value Object
+}
 
 type Environment struct {
-	names    [inlineCapacity]string
-	values   [inlineCapacity]Object
-	count    int
+	// Tier one: inline, so an environment this small costs no allocation
+	// beyond the struct itself.
+	names  [inlineCapacity]string
+	values [inlineCapacity]Object
+	count  int
+
+	// Tier two: allocated only for environments that outgrow the inline array.
+	// One slice of pairs rather than parallel slices, because the header sits
+	// in every environment, including the call frames that never grow one.
+	extra []binding
+
+	// Tier three: for environments larger than scanLimit. Once this exists the
+	// scan tiers are empty.
 	overflow map[string]Object
 
 	outer     *Environment
@@ -49,6 +77,12 @@ func (environment *Environment) local(name string) (Object, bool) {
 		}
 	}
 
+	for index := range environment.extra {
+		if environment.extra[index].name == name {
+			return environment.extra[index].value, true
+		}
+	}
+
 	if environment.overflow != nil {
 		value, ok := environment.overflow[name]
 
@@ -60,12 +94,16 @@ func (environment *Environment) local(name string) (Object, bool) {
 
 // All returns a copy of the names bound in this environment.
 func (environment *Environment) All() map[string]Object {
-	all := make(map[string]Object, environment.count+len(environment.overflow))
+	all := make(map[string]Object, environment.count+len(environment.extra)+len(environment.overflow))
 
 	for index := 0; index < environment.count; index++ {
 		all[environment.names[index]] = environment.values[index]
 	}
 
+	for index := range environment.extra {
+		all[environment.extra[index].name] = environment.extra[index].value
+	}
+
 	for name, value := range environment.overflow {
 		all[name] = value
 	}
@@ -119,14 +157,20 @@ func (environment *Environment) Set(name string, value Object) Object {
 		}
 	}
 
-	if environment.overflow != nil {
-		if _, ok := environment.overflow[name]; ok {
-			environment.overflow[name] = value
+	for index := range environment.extra {
+		if environment.extra[index].name == name {
+			environment.extra[index].value = value
 
 			return value
 		}
 	}
 
+	if environment.overflow != nil {
+		environment.overflow[name] = value
+
+		return value
+	}
+
 	if environment.count < inlineCapacity {
 		environment.names[environment.count] = name
 		environment.values[environment.count] = value
@@ -135,23 +179,43 @@ func (environment *Environment) Set(name string, value Object) Object {
 		return value
 	}
 
-	if environment.overflow == nil {
-		environment.overflow = make(map[string]Object)
+	if environment.count+len(environment.extra) < scanLimit {
+		environment.extra = append(environment.extra, binding{name: name, value: value})
+
+		return value
+	}
+
+	// Too large to scan. Move every binding into the map and abandon the scan
+	// tiers so that lookups cost one map access rather than a scan and a map
+	// access.
+	environment.overflow = make(map[string]Object, scanLimit*2)
+
+	for index := 0; index < environment.count; index++ {
+		environment.overflow[environment.names[index]] = environment.values[index]
+		environment.names[index] = ""
+		environment.values[index] = nil
+	}
+
+	for index := range environment.extra {
+		environment.overflow[environment.extra[index].name] = environment.extra[index].value
 	}
 
+	environment.count = 0
+	environment.extra = nil
+
 	environment.overflow[name] = value
 
 	return value
 }
 
 func (environment *Environment) Delete(name string) {
+	// Order is not meaningful in either scan tier, so removals close the gap
+	// with the last entry and clear it so the removed value can be collected.
 	for index := 0; index < environment.count; index++ {
 		if environment.names[index] != name {
 			continue
 		}
 
-		// Order is not meaningful here, so close the gap with the last entry
-		// and clear it so the removed value can be collected.
 		last := environment.count - 1
 
 		environment.names[index] = environment.names[last]
@@ -163,6 +227,20 @@ func (environment *Environment) Delete(name string) {
 		return
 	}
 
+	for index := range environment.extra {
+		if environment.extra[index].name != name {
+			continue
+		}
+
+		last := len(environment.extra) - 1
+
+		environment.extra[index] = environment.extra[last]
+		environment.extra[last] = binding{}
+		environment.extra = environment.extra[:last]
+
+		return
+	}
+
 	delete(environment.overflow, name)
 }