Skip to content

feat: add feature-gated profiler spans - #817

Merged
aram-devdocs merged 3 commits into
mainfrom
codex/issue-744-profiling-integration
Jul 8, 2026
Merged

feat: add feature-gated profiler spans#817
aram-devdocs merged 3 commits into
mainfrom
codex/issue-744-profiling-integration

Conversation

@aram-devdocs

Copy link
Copy Markdown
Owner

Overview

Type: feature

Summary:
Adds feature-gated Tracy/Puffin profiling support and spans for the WGPU frame path plus ECS schedule execution. Profiling is off by default and remains separate from fixed ENG2 phase counters.

Related Issues: Fixes #744


Changes Made

Engine Core (goud_engine/src/)

  • Added profiling-tracy and profiling-puffin optional features/dependencies.
  • Added a private libs::profiling helper so profiler backend selection is centralized and compiles out when disabled.
  • Instrumented WGPU frame phases and ECS schedule/system execution with named profiler spans.

FFI Layer (goud_engine/src/ffi/)

No changes.

C# SDK (sdks/csharp/)

No changes.

Python SDK (sdks/python/)

No changes.

TypeScript SDK (sdks/typescript/)

  • Node napi binding changes
  • Web WASM binding changes
  • Type definition changes
  • Tests updated
    No changes.

Codegen Pipeline (codegen/)

  • Schema changes
  • Generator changes
  • Validator changes
  • ffi_mapping changes
    No changes; ./codegen.sh was run and produced no generated drift.

Proc Macros (goud_engine_macros/)

  • #[goud_api] attribute changes
    No changes.

Tools (tools/)

  • lint-layers changes
    No changes.

WASM (goud_engine/src/wasm/)

  • wasm-bindgen exports
  • Sprite renderer changes
  • Texture loader changes
    No changes.

Examples (examples/)

No changes.

Documentation

  • Added docs/src/development/profiling.md with backend usage, viewer notes, span names, and feature check commands.
  • Added the profiling page to the docs summary.

Architectural Compliance

  • Rust-first: All logic lives in Rust; SDKs are thin wrappers
  • FFI boundary: No FFI exports or signatures changed
  • Dependency flow: Imports follow layer hierarchy (down only)
  • SDK parity: No FFI surface change; ./codegen.sh run
  • Unsafe discipline: No new unsafe blocks
  • File size: No file exceeds 500 lines

Testing

  • cargo test passes locally with GOUD_SKIP_NATIVE_SMOKE=1 because this machine has no usable native display
  • cargo clippy -- -D warnings is clean
  • cargo fmt --all -- --check passes
  • Python SDK tests pass (python3 sdks/python/test_bindings.py) — if SDK changed
  • C# SDK tests pass (dotnet test sdks/csharp.tests/) — if SDK changed
  • TypeScript SDK tests pass (cd sdks/typescript && npm test) — if TS SDK changed
  • Codegen produces consistent output (./codegen.sh, scripts/check-generated-artifacts.sh)
  • Pre-commit-equivalent local checks pass; see verification list below

Additional verification run:

cargo check
cargo check --features profiling-tracy
cargo check --features profiling-puffin
cargo fmt --all -- --check
cargo clippy -- -D warnings
cargo clippy --features profiling-tracy -- -D warnings
cargo clippy --features profiling-puffin -- -D warnings
cargo check --workspace --all-targets --all-features
cargo clippy --workspace --all-targets --all-features -- -D warnings
cargo test --test spec -- --nocapture
cargo test --features profiling-tracy --test spec -- --nocapture
cargo test --features profiling-puffin --test spec -- --nocapture
cargo test --all-features --test spec -- --nocapture
cargo test --all-features -p goud-engine-core --lib libs::profiling -- --nocapture
cargo test --features profiling-puffin -p goud-engine-core --lib libs::profiling -- --nocapture
GOUD_SKIP_NATIVE_SMOKE=1 cargo test
cargo check -p sandbox --features goud_engine/profiling-tracy
cargo check -p sandbox --features goud_engine/profiling-puffin
cargo deny check advisories
./codegen.sh
scripts/check-generated-artifacts.sh
scripts/check-rs-line-limit.sh
git diff --check
git diff --exit-code

Code Quality

  • No todo!() or unimplemented!() in production code
  • No #[allow(unused)] without justification comment
  • Error handling uses Result, not unwrap()/expect() in library code
  • Public items have doc comments or this change adds no public API surface

Documentation

  • Updated relevant developer docs
  • Updated relevant AGENTS.md files (if architecture changed)
  • Updated README.md (if user-facing behavior changed)
  • Added or updated doc comments on new public APIs, or no new public APIs were added

Breaking Changes

None.

  • API changes: None
  • FFI signature changes: None
  • SDK interface changes: None

Version Bump

Bump type: none
Justification: Internal feature-gated developer instrumentation only.


Security

  • No new unsafe blocks — or each one has a // SAFETY: comment and is necessary
  • No new FFI pointer parameters without null checks
  • No new dependencies with known advisories (cargo deny check advisories)
  • No secrets or credentials in committed files

Performance

Default builds do not enable the profiler backends. The span sites compile out unless profiling-tracy or profiling-puffin is enabled. This is developer instrumentation, not a perf gate capture.


Deployment

  • NuGet package version updated (if SDK changed)
  • Python package version updated (if SDK changed)
  • npm package version updated (if TS SDK changed)
  • Native library builds on local target via compile/check gates above

Reviewer Notes

  • Architecture gate validated the private helper placement and layer direction; no FFI/generated SDK boundary changes were introduced.
  • Code-quality gate approved after replacing brittle source-text spec checks with contract-level spec tests and private helper unit tests.
  • Plain local cargo test previously reaches the native present path and fails on this no-display machine; GOUD_SKIP_NATIVE_SMOKE=1 cargo test passed and the skipped smoke tests report no display available.

@github-actions github-actions Bot added the size/L label Jul 8, 2026
@claude

claude Bot commented Jul 8, 2026

Copy link
Copy Markdown
Contributor

Claude finished @aram-devdocs's task in 3m 57s —— View job


Code Review — feat: add feature-gated profiler spans

Verdict: APPROVED

Clean, well-scoped, feature-gated instrumentation. Compiles out entirely by default, doesn't touch FFI/SDK surfaces, and the spec test verifies plumbing at the cargo metadata/cargo tree level rather than grepping source text. Two small non-blocking observations below.

Phase 2–3: Architecture & Anti-Patterns

  • Layering is correct: libs::profiling (goud_engine/src/libs/profiling.rs) is pub(crate), declared in goud_engine/src/libs/mod.rs:6, and consumed downward from ecs/schedule/* (Layer 3) and libs/graphics/backend/wgpu_backend/frame.rs (same layer, cross-cutting utility like libs::error). No upward imports, no FFI/SDK touched.
  • No raw GPU calls added outside backend/; no unsafe, no FFI signatures changed — matches the PR description.
  • No unwrap()/expect()/todo!() introduced in library code.
  • All files stay well under the 500-line cap (largest is system_stage.rs at 462 lines, frame.rs at 447).
  • Span names in docs/src/development/profiling.md match the emit sites exactly (wgpu.begin_frameecs.parallel_system), verified against frame.rs, execution.rs, system_stage.rs.

WARNINGS (non-blocking)

# File:Line Issue
1 goud_engine/src/libs/graphics/backend/wgpu_backend/frame.rs:12-13 crate::libs::profiling::begin_frame!() calls puffin::set_scopes_on(true), but the very next line's profile_scope!(WGPU_BEGIN_FRAME) already does the same thing internally on every scope invocation (profiling.rs:58,67). The explicit begin_frame!() call is redundant — it never has an effect that the following profile_scope! wouldn't already produce. Low severity (cheap atomic store), but worth simplifying so the macro's purpose isn't misleading. Fix this →
2 goud_engine/src/libs/graphics/backend/wgpu_backend/frame.rs:66 end_frame()'s early return (self.current_frame.take().ok_or(...)?) does not call crate::libs::profiling::finish_frame!(), unlike the readback-error path at lines 289-291 which explicitly closes the profiler frame before propagating its error. If this "no active frame" invariant is ever hit while a profiling feature is enabled, the Tracy/Puffin frame boundary silently rolls into the next frame. Rare edge case, but inconsistent with the pattern established elsewhere in the same function. Fix this →

Scope note (informational, not blocking)

deny.toml drops the RUSTSEC-2026-0186 (memmap2) and RUSTSEC-2026-0189 (rmcp) ignore entries. This isn't mentioned in the PR description, and this PR's Cargo.lock diff doesn't touch memmap2 or rmcp — rmcp was already bumped to 1.5.0 in a prior, separate PR (#691), which is presumably why the ignore entry is now stale/unneeded. The cleanup itself looks correct, just undocumented and unrelated to the profiling feature — worth a one-line mention in the PR description for traceability.

Positive callouts

  • Spec test (goud_engine/tests/spec/eng2_p0_04_profiling_integration.rs) checks contracts via cargo metadata/cargo tree output rather than brittle string matching against source — verifies profiling-tracy/profiling-puffin stay out of default, and that tracy-client/tracy-client-sys/puffin are absent from the default dependency graph.
  • Unit tests in profiling.rs cover all three meaningful states: Tracy-only, Puffin-only, and both-enabled (correctly asserts Tracy wins and Puffin stays off), including the documented invariant that ECS puffin scopes work without ever entering the WGPU frame path.
  • Doc (docs/src/development/profiling.md) is concise, lists every span, states the default-build cost clearly, and gives compile-only validation commands for both backends.

@github-actions github-actions Bot added size/L and removed size/L labels Jul 8, 2026
@aram-devdocs

Copy link
Copy Markdown
Owner Author

Follow-up pushed in 93127fe: addressed both non-blocking Claude warnings by removing the redundant Puffin begin-frame helper/call and closing the profiler frame before the WGPU no-active-frame early return. Also noting for traceability: the deny.toml changes remove stale advisory ignores that are no longer needed with the current lockfile.

@github-actions github-actions Bot added size/L and removed size/L labels Jul 8, 2026
@aram-devdocs
aram-devdocs force-pushed the codex/issue-744-profiling-integration branch from 93127fe to a47b37f Compare July 8, 2026 20:16
@github-actions github-actions Bot added size/L and removed size/L labels Jul 8, 2026
@codecov

codecov Bot commented Jul 8, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.

📢 Thoughts on this report? Let us know!

@aram-devdocs
aram-devdocs enabled auto-merge (squash) July 8, 2026 20:43
@aram-devdocs
aram-devdocs force-pushed the codex/issue-744-profiling-integration branch from a47b37f to 027ba50 Compare July 8, 2026 21:22
@github-actions github-actions Bot added size/L and removed size/L labels Jul 8, 2026
@aram-devdocs
aram-devdocs merged commit b10f727 into main Jul 8, 2026
54 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

ENG2-P0-04: Feature-gated tracy/puffin profiler integration

1 participant