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 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) }