perf: overlap wait_for_output_drain with render work

[RtlZeroMemory]

Summary

• move wait_for_output_drain wait from the top of engine_present() to just before flush
• gate wait on out_len != 0 so no-byte presents are not blocked by output-drain pacing
• preserve conservative hash-reuse invalidation on wait failure before commit
• add unit coverage for the no-flush wait skip behavior

Why

The old flow always waited before doing diff/render work, serializing potential terminal drain latency with CPU work. Reordering lets diff/render run first and only waits at flush point, preserving draw semantics while improving overlap.

Tests

• cmake --preset posix-clang-debug -DZIREAEL_WARNINGS_AS_ERRORS=ON -DZIREAEL_BUILD_EXAMPLES=OFF
• cmake --build --preset posix-clang-debug
• ctest --test-dir out/build/posix-clang-debug --no-tests=ignore --output-on-failure
• cmake --preset posix-clang-asan-ubsan -DZIREAEL_WARNINGS_AS_ERRORS=ON -DZIREAEL_BUILD_EXAMPLES=OFF
• cmake --build --preset posix-clang-asan-ubsan
• ctest --test-dir out/build/posix-clang-asan-ubsan --no-tests=ignore --output-on-failure

All tests passed (12/12 on both lanes).

Summary by CodeRabbit

• Bug Fixes

  • Improved output handling error recovery with more conservative state management during write failures.

• Performance

  • Optimized output drain waiting logic to skip unnecessary waits when no data needs to be flushed.

• Tests

  • Added test coverage for output drain backpressure scenarios.

[coderabbitai]

📝 Walkthrough

Walkthrough

The output drain wait mechanism in engine_present was refactored to execute conditionally after diff/render operations instead of pre-emptively, gated by output length and runtime configuration. Error handling was adjusted to reset prev-hash validation state before returning on failure.

Changes

Cohort / File(s)	Summary
Core Engine Output Drain Wait Logic src/core/zr_engine_present.inc	Repositioned wait-for-output-drain check to occur after diff/render but before write operations, now conditional on out_len != 0 and cfg_runtime.wait_for_output_drain. Both wait-writable and write failures now mark diff_prev_hashes_valid = 0 for conservative state reset before error return.
Output Drain Backpressure Test tests/unit/test_engine_present_backpressure.c	Added test verifying that wait-for-output-drain is skipped when no flush is needed (out_len == 0), confirming zero bytes written and that prior wait count remains unchanged.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A wait that moved, from start to end,
Now skips when nothing needs to send,
The hash resets when troubles brew,
Conservative and ever true! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (2 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title accurately describes the main performance optimization: moving wait_for_output_drain to overlap with render work instead of blocking at the start.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

• 📝 Generate docstrings (stacked PR)
• 📝 Generate docstrings (commit on current branch)

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Post copyable unit tests in a comment
• Commit unit tests in branch perf/present-wait-overlap

---

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

🧹 Nitpick comments (1)

tests/unit/test_engine_present_backpressure.c (1)

85-114: Add explicit Arrange/Act/Assert section markers for test structure consistency.

Please use the standardized section comments in this test body.

Proposed refactor

 ZR_TEST_UNIT(engine_present_wait_for_output_drain_skips_wait_when_no_flush_needed) {
+  /* --- Arrange --- */
   mock_plat_reset();
   mock_plat_set_size(10u, 4u);
   mock_plat_set_output_writable(1u);

   zr_engine_config_t cfg = zr_engine_config_default();
   cfg.limits.out_max_bytes_per_frame = 4096u;
   cfg.wait_for_output_drain = 1u;

   zr_engine_t* e = NULL;
   ZR_ASSERT_EQ_U32(engine_create(&e, &cfg), ZR_OK);
   ZR_ASSERT_TRUE(e != NULL);

+  /* --- Act --- */
   ZR_ASSERT_EQ_U32(engine_submit_drawlist(e, zr_test_dl_fixture1, (int)zr_test_dl_fixture1_len), ZR_OK);
   ZR_ASSERT_EQ_U32(engine_present(e), ZR_OK);
   ZR_ASSERT_EQ_U32(mock_plat_wait_output_call_count(), 1u);

   /*
     No drawlist/event changes for the second present => no terminal flush bytes.
     wait_for_output_drain should not gate this no-op frame.
   */
   mock_plat_clear_writes();
   mock_plat_set_output_writable(0u);

   ZR_ASSERT_EQ_U32(engine_present(e), ZR_OK);
+  /* --- Assert --- */
   ZR_ASSERT_EQ_U32(mock_plat_wait_output_call_count(), 1u);
   ZR_ASSERT_EQ_U32((uint32_t)mock_plat_bytes_written_total(), 0u);

   engine_destroy(e);
 }

As per coding guidelines: Add section markers in long functions (/* --- Section Name --- */) and in tests (/* --- Arrange/Act/Assert --- */, /* --- Validate/Compute/Emit --- */).

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@tests/unit/test_engine_present_backpressure.c` around lines 85 - 114, Add the
standardized test section markers to the test function
engine_present_wait_for_output_drain_skips_wait_when_no_flush_needed: insert a
/* --- Arrange/Act/Assert --- */ comment before setup calls (mock_plat_reset..
cfg/engine_create), add a marker between the actions
(engine_submit_drawlist/engine_present) and the no-op second-present setup to
split Act from Assert, and optionally add /* --- Validate/Compute/Emit --- */
(or the project's preferred validate marker) before final assertions
(mock_plat_wait_output_call_count... engine_destroy) so the test clearly shows
Arrange, Act, and Assert sections around the calls to engine_submit_drawlist,
engine_present, mock_plat_clear_writes, and subsequent assertions.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In `@tests/unit/test_engine_present_backpressure.c`:
- Around line 85-114: Add the standardized test section markers to the test
function engine_present_wait_for_output_drain_skips_wait_when_no_flush_needed:
insert a /* --- Arrange/Act/Assert --- */ comment before setup calls
(mock_plat_reset.. cfg/engine_create), add a marker between the actions
(engine_submit_drawlist/engine_present) and the no-op second-present setup to
split Act from Assert, and optionally add /* --- Validate/Compute/Emit --- */
(or the project's preferred validate marker) before final assertions
(mock_plat_wait_output_call_count... engine_destroy) so the test clearly shows
Arrange, Act, and Assert sections around the calls to engine_submit_drawlist,
engine_present, mock_plat_clear_writes, and subsequent assertions.

---

ℹ️ Review info

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between e0729e4 and 77539e6.

📒 Files selected for processing (2)

• src/core/zr_engine_present.inc
• tests/unit/test_engine_present_backpressure.c