Skip to content

perf: keep block resolution off the heap - #16

Merged
HashimTheArab merged 1 commit into
oomph-ac:mainfrom
oriumgames:perf/block-resolve-escape
Aug 16, 2026
Merged

perf: keep block resolution off the heap#16
HashimTheArab merged 1 commit into
oomph-ac:mainfrom
oriumgames:perf/block-resolve-escape

Conversation

@schphe

@schphe schphe commented Aug 12, 2026

Copy link
Copy Markdown
Contributor

Resolve allocates on every call. rule.Apply takes a pointer, and passing
&result into an interface method makes escape analysis give up and stick it
on the heap.

Changed Apply to take and return the resolution by value. Takes a tick from
47 allocations down to 35. No behaviour change.

Summary by CodeRabbit

  • Refactor
    • Updated movement and block-resolution processing to use returned values consistently.
    • Preserved existing behavior for bouncing, climbing, cobwebs, friction, environmental effects, and Soul Speed.
  • Tests
    • Updated resolution-processing test coverage to match the revised internal handling.

rule.Apply took *resolution, so passing &result to an interface method
defeated escape analysis and heap-allocated the resolution on every
Resolve call. Resolve runs several times per movement tick, which made
this 23% of all allocations in a tick.

Pass and return the resolution by value instead. Behaviour is unchanged:
the trajectory checksum over a 64-yaw sweep is identical.

Co-Authored-By: Claude <noreply@anthropic.com>
@coderabbitai

coderabbitai Bot commented Aug 12, 2026

Copy link
Copy Markdown

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: e8e694cf-614b-418d-b2fd-a1add90facf6

📥 Commits

Reviewing files that changed from the base of the PR and between 21874fd and ce6be63.

📒 Files selected for processing (8)
  • block/bounce.go
  • block/climbable.go
  • block/contact.go
  • block/environment.go
  • block/friction.go
  • block/ground.go
  • block/semantics.go
  • block/semantics_test.go

📝 Walkthrough

Walkthrough

The block semantics API now passes resolution values into Apply methods and receives updated values in return. Resolve propagates each returned resolution through matching rules. Existing block behavior remains unchanged.

Changes

Resolution value flow

Layer / File(s) Summary
Rule contract and resolution propagation
block/semantics.go
The internal rule.Apply contract returns resolution values. Resolve assigns each returned resolution while processing matching rules.
Block handlers and test rule
block/bounce.go, block/climbable.go, block/contact.go, block/environment.go, block/friction.go, block/ground.go, block/semantics_test.go
Block handlers and countingRule now use value-based resolution input and output while preserving existing field updates.

Estimated code review effort: 2 (Simple) | ~10 minutes

Possibly related PRs

Suggested reviewers: hashimthearab

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (4 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title clearly and concisely describes the main change: avoiding heap allocation for block resolution.
Linked Issues check ✅ Passed Check skipped because no linked issues were found for this pull request.
Out of Scope Changes check ✅ Passed Check skipped because no linked issues were found for this pull request.
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@HashimTheArab
HashimTheArab merged commit b55c950 into oomph-ac:main Aug 16, 2026
1 check passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants