perf: environment scan tier, plus CI that can actually fail - #142
Merged
Conversation
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.
`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.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What does this implement or fix?
Follow-up to #140. Two independent pieces: one performance fix for a workload shape the merged work regressed, and the CI gaps that let related breakage through unnoticed.
Added
Environment, between the inline array and the map. Bindings are looked up in a fixed inline array, then a slice that only exists for environments that outgrow it, then a map pastscanLimit.TileRenderbenchmark: many globals in one flat scope read from a tight nested loop, with no function calls. The suite previously had no flat-scope case, which is why the regression below was missed.bench,fmt,vetandchecktargets in the Makefile, and matching CI steps.Fixed
make testpipesgo testthroughsedfor colour. A pipeline reports the exit status of its last command, sosedalways succeeded and the target exited 0 no matter how the tests went — and the workflow runsmake test. Verified by making a test fail and watching the exit code go from 0 to 1 once the target runs under bash withpipefail.ClassMethodsstill usedCounter.new(), which Adopt JavaScript-style class syntax and fix OOP semantics #141 removed. Benchmark bodies are not compiled into a plaingo testrun and the Ghost programs inside them are only parsed when they execute, so tests stayed green while the suite was broken. Updated tonew Counter(), andmake benchnow runs each benchmark for a single iteration in CI so this cannot recur silently.actions/checkoutandactions/setup-gomove from v2 (retired Node runtime) to v4/v5; the Go version comes fromgo.modrather than the^1.17that had drifted from the declared 1.21.1;pull_requestis no longer filtered to the1.0base, so a PR is checked wherever it is targeted; the test timeout goes from 5s to 120s, since under-raceon a cold runner the existing tests approach the old limit and risked flaking.Removed
dep/Gopkg.tomlbootstrap block in the workflow. There is noGopkg.tomland modules have handled this since the repository moved to them.Does this close any currently open issues?
None.
Additional Notes
On the performance number. The ~8% figure was measured against the pre-#141 tree. The class rework touched the evaluator enough that it is worth re-measuring on this base rather than carrying the number over;
make benchis the starting point.Why scanning wins. More than it looks like it should: the 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
scanLimita scan genuinely stops paying, so everything migrates into the map and the scan tiers are abandoned — splitting names between a scan tier and a map is worse than either alone, which is exactly the bug being fixed. 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.Behaviour.
Get,Set,Has,Delete,All,HasLocalandGetLocalkeep their existing semantics; only the storage behind them changes.Verification limits.
gofmt,go vet,go build,go test -raceand the benchmarks pass, but on every package exceptrepl, whoselinerdependency could not be fetched in the environment this was prepared in. CI covers the full./..., and it is now able to report a failure if that is wrong.Two separable commits. The CI fix is independent of the perf change and matters on its own, since the benchmark suite is broken on
1.0today and CI is not reporting it. Happy to split it out if you would rather land that first.Screenshots
N/A — no visual change.
🤖 Generated with Claude Code
https://claude.ai/code/session_01XtRdJPCskcS2ryiQZoEbUx
Generated by Claude Code