Add: port a2a3 eager prewarm arena to a5 tmr#1451
Open
yanghaoran29 wants to merge 1 commit into
Open
Conversation
- Align a5 runtime_maker with a2a3 hw-native-sys#1322: shared build_and_cache_prebuilt_arena, strong prewarm_config_impl, decouple ensure_static_arenas from Runtime - Add a5 prewarm regression test and onboard cold/hot benchmark Onboard effect (a5, ring_task_window=64, dummy_task): cold first-run bind.prebuilt ~9.58 ms -> hot ~0.002 ms; ~9.97 ms build cost shifts to init simpler_prewarm.build (cache hit on first run).
📝 WalkthroughWalkthroughThe runtime arena miss path now uses a shared build/upload/cache helper, while cache hits and newly built arenas are wired directly. A new ChangesRuntime arena prewarming
Estimated code review effort: 4 (Complex) | ~45 minutes Sequence Diagram(s)sequenceDiagram
participant Worker
participant prewarm_config_impl
participant build_and_cache_prebuilt_arena
participant RuntimeArenaCache
Worker->>prewarm_config_impl: provide ring sizing configuration
prewarm_config_impl->>build_and_cache_prebuilt_arena: build and upload arena image
build_and_cache_prebuilt_arena->>RuntimeArenaCache: store prebuilt runtime image
Worker->>RuntimeArenaCache: bind configured runtime
RuntimeArenaCache-->>Worker: return cached arena metadata
Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
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. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/st/a5/tensormap_and_ringbuffer/test_prewarm_config.py (1)
19-36: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winSolid smoke test, but doesn't exercise the cache-hit path it's meant to protect.
This confirms
worker.init(prewarm_config=...)doesn't crash, but never follows up withregister/runusing matching ring sizing to confirm the prewarmed image is actually consumed on bind (i.e., that the cache-hit branch inbind_callable_to_runtime_implis reached rather than a silent fallback to the miss path).worker = Worker( level=2, device_id=int(st_device_ids[0]), platform=st_platform, runtime=_RUNTIME, ) try: worker.init(prewarm_config=config) + # Exercise a bind with matching ring sizing and assert it takes the + # cache-hit path (e.g. via a callable register/run + STRACE assertion). finally: worker.close()🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/st/a5/tensormap_and_ringbuffer/test_prewarm_config.py` around lines 19 - 36, Extend test_l2_init_with_prewarm_config to register and run a workload after worker.init using the same ring_task_window=64 configuration, ensuring the prewarmed image is consumed during binding. Exercise the bind_callable_to_runtime_impl cache-hit path and retain the existing worker cleanup.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@tests/st/a5/tensormap_and_ringbuffer/bench_prewarm_timing.py`:
- Around line 165-184: Make all timing output in the displayed per-run and
“both” summary paths None-safe. Reuse the existing formatting approach from
prewarm_text for parsed bind_prebuilt_strace_ms, cold, hot, and prewarm_ms
values, ensuring missing timings print a fallback such as “None” instead of
applying :.2f; preserve the existing guarded savings calculations.
---
Nitpick comments:
In `@tests/st/a5/tensormap_and_ringbuffer/test_prewarm_config.py`:
- Around line 19-36: Extend test_l2_init_with_prewarm_config to register and run
a workload after worker.init using the same ring_task_window=64 configuration,
ensuring the prewarmed image is consumed during binding. Exercise the
bind_callable_to_runtime_impl cache-hit path and retain the existing worker
cleanup.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: 07025a8b-2d83-4aa5-8494-16509183e5af
📒 Files selected for processing (3)
src/a5/runtime/tensormap_and_ringbuffer/host/runtime_maker.cpptests/st/a5/tensormap_and_ringbuffer/bench_prewarm_timing.pytests/st/a5/tensormap_and_ringbuffer/test_prewarm_config.py
Comment on lines
+165
to
+184
| prewarm_val = parsed["simpler_prewarm_build_ms"] | ||
| prewarm_text = f"{prewarm_val:.2f}ms" if prewarm_val is not None else "None" | ||
| print( | ||
| f" -> bind.prebuilt(STRACE)={parsed['bind_prebuilt_strace_ms']:.2f}ms" | ||
| f", prewarm.build={prewarm_text}", | ||
| flush=True, | ||
| ) | ||
|
|
||
| if args.mode == "both": | ||
| cold = results["cold"]["bind_prebuilt_strace_ms"] | ||
| hot = results["hot"]["bind_prebuilt_strace_ms"] | ||
| prewarm_ms = results["hot"]["simpler_prewarm_build_ms"] | ||
| print("\n=== SUMMARY ===") | ||
| print(f"cold first-run bind.prebuilt (STRACE) : {cold:.2f} ms") | ||
| print(f"hot first-run bind.prebuilt (STRACE) : {hot:.2f} ms") | ||
| print(f"hot init simpler_prewarm.build : {prewarm_ms:.2f} ms") | ||
| if cold is not None and hot is not None: | ||
| print(f"first-run savings (cold - hot) : {cold - hot:.2f} ms") | ||
| if prewarm_ms is not None: | ||
| print(f"(build cost shifted to init prewarm) : ~{prewarm_ms:.2f} ms") |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Unguarded .2f formatting on possibly-None timing values.
_parse_log types bind_prebuilt_strace_ms/simpler_prewarm_build_ms as float | None, and line 166 already guards against None for prewarm_text, but lines 168, 178, 179, and 180 format these values directly with :.2f with no None check. If a TIMING/STRACE log line fails to match (e.g. log-level or format drift), this raises TypeError and the benchmark crashes without reporting partial results, unlike the guarded delta computation at 181-184.
🐛 Proposed fix for None-safe formatting
+ def _fmt(v: float | None) -> str:
+ return f"{v:.2f}ms" if v is not None else "None"
+
prewarm_val = parsed["simpler_prewarm_build_ms"]
prewarm_text = f"{prewarm_val:.2f}ms" if prewarm_val is not None else "None"
print(
- f" -> bind.prebuilt(STRACE)={parsed['bind_prebuilt_strace_ms']:.2f}ms"
+ f" -> bind.prebuilt(STRACE)={_fmt(parsed['bind_prebuilt_strace_ms'])}"
f", prewarm.build={prewarm_text}",
flush=True,
)
if args.mode == "both":
cold = results["cold"]["bind_prebuilt_strace_ms"]
hot = results["hot"]["bind_prebuilt_strace_ms"]
prewarm_ms = results["hot"]["simpler_prewarm_build_ms"]
print("\n=== SUMMARY ===")
- print(f"cold first-run bind.prebuilt (STRACE) : {cold:.2f} ms")
- print(f"hot first-run bind.prebuilt (STRACE) : {hot:.2f} ms")
- print(f"hot init simpler_prewarm.build : {prewarm_ms:.2f} ms")
+ print(f"cold first-run bind.prebuilt (STRACE) : {_fmt(cold)}")
+ print(f"hot first-run bind.prebuilt (STRACE) : {_fmt(hot)}")
+ print(f"hot init simpler_prewarm.build : {_fmt(prewarm_ms)}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| prewarm_val = parsed["simpler_prewarm_build_ms"] | |
| prewarm_text = f"{prewarm_val:.2f}ms" if prewarm_val is not None else "None" | |
| print( | |
| f" -> bind.prebuilt(STRACE)={parsed['bind_prebuilt_strace_ms']:.2f}ms" | |
| f", prewarm.build={prewarm_text}", | |
| flush=True, | |
| ) | |
| if args.mode == "both": | |
| cold = results["cold"]["bind_prebuilt_strace_ms"] | |
| hot = results["hot"]["bind_prebuilt_strace_ms"] | |
| prewarm_ms = results["hot"]["simpler_prewarm_build_ms"] | |
| print("\n=== SUMMARY ===") | |
| print(f"cold first-run bind.prebuilt (STRACE) : {cold:.2f} ms") | |
| print(f"hot first-run bind.prebuilt (STRACE) : {hot:.2f} ms") | |
| print(f"hot init simpler_prewarm.build : {prewarm_ms:.2f} ms") | |
| if cold is not None and hot is not None: | |
| print(f"first-run savings (cold - hot) : {cold - hot:.2f} ms") | |
| if prewarm_ms is not None: | |
| print(f"(build cost shifted to init prewarm) : ~{prewarm_ms:.2f} ms") | |
| def _fmt(v: float | None) -> str: | |
| return f"{v:.2f}ms" if v is not None else "None" | |
| prewarm_val = parsed["simpler_prewarm_build_ms"] | |
| prewarm_text = f"{prewarm_val:.2f}ms" if prewarm_val is not None else "None" | |
| print( | |
| f" -> bind.prebuilt(STRACE)={_fmt(parsed['bind_prebuilt_strace_ms'])}" | |
| f", prewarm.build={prewarm_text}", | |
| flush=True, | |
| ) | |
| if args.mode == "both": | |
| cold = results["cold"]["bind_prebuilt_strace_ms"] | |
| hot = results["hot"]["bind_prebuilt_strace_ms"] | |
| prewarm_ms = results["hot"]["simpler_prewarm_build_ms"] | |
| print("\n=== SUMMARY ===") | |
| print(f"cold first-run bind.prebuilt (STRACE) : {_fmt(cold)}") | |
| print(f"hot first-run bind.prebuilt (STRACE) : {_fmt(hot)}") | |
| print(f"hot init simpler_prewarm.build : {_fmt(prewarm_ms)}") | |
| if cold is not None and hot is not None: | |
| print(f"first-run savings (cold - hot) : {cold - hot:.2f} ms") | |
| if prewarm_ms is not None: | |
| print(f"(build cost shifted to init prewarm) : ~{prewarm_ms:.2f} ms") |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/st/a5/tensormap_and_ringbuffer/bench_prewarm_timing.py` around lines
165 - 184, Make all timing output in the displayed per-run and “both” summary
paths None-safe. Reuse the existing formatting approach from prewarm_text for
parsed bind_prebuilt_strace_ms, cold, hot, and prewarm_ms values, ensuring
missing timings print a fallback such as “None” instead of applying :.2f;
preserve the existing guarded savings calculations.
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
1 participant
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.
Onboard effect (a5, ring_task_window=64, dummy_task): cold first-run bind.prebuilt ~9.58 ms -> hot ~0.002 ms; ~9.97 ms build cost shifts to init simpler_prewarm.build (cache hit on first run).