From 21910aa8c180b6538b000b5e74b1e4341cba31ba Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 05:52:06 +0000 Subject: [PATCH 1/2] perf: give environments a scan tier between the inline array and the map A tile rendering loop - many globals in one flat scope, read from a tight nested loop with no function calls - was 15% slower after environments moved to inline storage, even though call-heavy benchmarks got much faster. With four inline slots and a map behind them, a scope holding nine globals kept its first four inline and put the rest in the map, so reading any of those five paid a full scan and then a hash. Raising the inline capacity fixes that scope and penalizes every call frame, because the array sits in a struct that is allocated on every call. The two shapes of scope want opposite sizes, so there is now a middle tier: a slice that only exists for environments that outgrow the inline array, scanned before falling back to the map. Past scanLimit everything migrates into the map and the scan tiers are abandoned, since splitting names between a scan tier and a map is worse than either alone. The tier is one slice of name/value pairs rather than two parallel slices; with two, the extra header made call-heavy benchmarks 10% slower even though they never allocate the tier. Scanning wins here by more than it appears it should: the name being looked up and the name stored usually come from the same AST identifier, so the comparison settles on equal pointers without reading characters. Tile rendering is 8% faster, loop and map benchmarks improve, and the call benchmarks are unchanged. The workload is now in the benchmark suite, which previously had no flat-scope case and so missed this. Also updates the ClassMethods benchmark to the `new Counter()` syntax. The benchmarks are not compiled by a plain `go test` run, so the class syntax change missed this file and the suite failed to parse. --- evaluator/benchmark_test.go | 35 ++++++++++- object/environment.go | 122 +++++++++++++++++++++++++++++------- 2 files changed, 133 insertions(+), 24 deletions(-) diff --git a/evaluator/benchmark_test.go b/evaluator/benchmark_test.go index 8b87e03..048edc8 100644 --- a/evaluator/benchmark_test.go +++ b/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} diff --git a/object/environment.go b/object/environment.go index 6ab59f2..6c8e190 100644 --- a/object/environment.go +++ b/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) } From 44d8e4b383737ed7ab8cc537ab53037a1d3eb857 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 21 Aug 2026 18:51:49 +0000 Subject: [PATCH 2/2] ci: make the workflow able to fail, and cover vet, fmt and benchmarks `make test` pipes go test through sed for colour. A pipeline reports the exit status of its last command, so the sed always succeeded and the target exited 0 no matter how the tests went. The workflow runs `make test`, so CI reported success for any failing test run. The target now runs under bash with pipefail; verified by making a test fail and watching the exit code go from 0 to 1. Benchmark bodies are not compiled into a plain `go test` run, and the Ghost programs inside them are only parsed once they execute. That is how the class syntax change left the benchmark suite unable to parse while every test still passed. A `bench` target runs each benchmark for a single iteration, which is enough to catch it. Also adds `fmt` and `vet` targets, and a `check` target that runs all four for local use. Vet is worth having in CI: it flags the `string(obj.Type())` conversions that the integer type change turned into one-rune strings. Workflow changes beyond the new steps: - actions/checkout and actions/setup-go move from v2 to v4/v5. The v2 releases run on a Node version GitHub has retired. - The Go version comes from go.mod rather than the `^1.17` in the workflow, which had drifted from the 1.21.1 the module declares. - The dep/Gopkg.toml bootstrap block is removed. There is no Gopkg.toml and modules have handled this since the repository moved to them. - pull_request is no longer filtered to the 1.0 base branch, so a pull request is checked wherever it is targeted. - The test timeout goes from 5s to 120s. Under -race on a cold runner the race benchmarks alone approach the old limit, so it risked flaking. --- .github/workflows/test.yml | 41 +++++++++++++++++++++++--------------- Makefile | 23 +++++++++++++++++++-- 2 files changed, 46 insertions(+), 18 deletions(-) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 4ae6f90..825eae6 100644 --- a/.github/workflows/test.yml +++ b/.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 diff --git a/Makefile b/Makefile index 210a14b..6f4c770 100644 --- a/Makefile +++ b/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