Repository navigation
Conversation
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
The happy-path backpressure test is useful, and I verified that the worker’s 100 ms timed wait drains a full queue even when it is below the normal 64 KiB notify threshold. I found an unhandled writer-error path in the new wait protocol:
run()swapspendingintobatchand notifiesspaceAvailable.- A waiting producer wakes and can fill
pendingback to the limit. - If
output.write()oroutput.flush()then throws, the catch callsstd::terminate()and the worker exits without notifyingspaceAvailableagain. - The producer’s predicate only checks queue size, so it can remain blocked forever; the destructor can also wait indefinitely if this occurs during shutdown.
The PR says file-write errors retain their existing behavior, but the new condition-variable wait adds a stranded-waiter case on that existing fatal path. Please either make writer failure a synchronized terminal predicate that wakes all waiters (and ensure shutdown handles it consistently), or demonstrate with a deterministic failing stream test that this state cannot occur. Please add a focused regression for writer failure; the current test only exercises a recovering sink. I have not modified your branch.
|
Other open pull requests touch the same code. Merging one will break the other, or both implement the same thing. If this one builds on them, list them in Depends on.
Some comparisons could not be completed because a pull request conflicts with the base branch. Review these shared files manually before concluding that there is no overlap.
|
|
@MarcZX10 Thanks for asking for this check. I added write-failure and flush-failure tests in 58afe45. Each test holds the writer, fills the queue, starts four producers, and checks that none can finish. It then makes the stream return a short write or a failed flush. To pass, the child must reach the real writer catch, call the saved original terminate handler, and exit with the same result as an abort control. A random crash or timeout does not count. All 11 capture-trace tests pass locally on Windows, and both new cases passed 10 consecutive runs. I also tried a temporary copy where the catch returns instead of calling terminate: both tests failed through the watchdog, with the producers still waiting. One clarification: There is a coverage limit: these are host-runtime component tests using the production header. With the project's current Windows This investigation and the new tests were AI-assisted with Codex. |
MarcZX10
left a comment
MarcZX10
left a comment
There was a problem hiding this comment.
Rechecked exact head 58afe4560099d805edf020ae603e84a5fec063d6. The added writer-failure tests address the stranded-producer concern: they fill the bounded queue, verify four producers remain blocked, then trigger both a short write and a failed flush through the production CaptureTrace::run() catch and require the saved terminate handler to abort. The fixture distinguishes setup failure, watchdog exit, handler return, and the abort control result. The PR also clearly limits this evidence: the failure target uses the host C++ runtime; the separate probe with the project's Windows libc did not reach the catch, and that path is not claimed as covered.
I independently compiled CaptureTrace.cpp and CaptureTraceFailure.cpp from this exact head on Linux with g++ -std=c++20 -Wall -Wextra -Werror -pthread. The queue-backpressure case passed, and the CMake failure harness passed both write and flush modes. tools/check_conventions.py --base upstream/main --head upstream/pr/10760 and git diff --check pass. Hosted conventions and labels pass; the product build/test jobs are skipped by path filters, so I am not claiming hosted product coverage. The author reports the full Windows library/relinker builds and focused Windows suite in the PR description.
|
Independent synthetic validation of
I compiled your pinned public header/fixtures as standalone host-runtime executables, with no AnyPS5 replacement libc or driver linkage. Scratch files were on our NAS, but the paused/failing sinks are synthetic buffers: this is not an SMB-throughput test, full configured PR build, game/GPU validation, or approval of the logging policy. Your documented possibility of stalled callers/shutdown remains a maintainer decision in #10753. No composition branch was changed for this check. AI-assisted: yes (Codex). |
|
@lgqdaxia #11157 (Linux frame capture) and this PR touch the same spot in I'm happy with either order: whichever merges second rebases and keeps both entries (yours first, then the rewritten "Frame capture" one). No Unrelated small thing I noticed while reading it: the entry is followed by an empty line, which splits the Functional list in two when rendered. |
|
Agreed, @janzezno. I checked #11157's changed files and patches: the production changes are independent, and the CMake test additions are in separate sections. Let's keep both PRs independent, with no I also removed the extra blank line in AI-assisted: Codex |
|
@lgqdaxia Thanks for picking this up. I've answered the policy question on #10753: a trace must never stop or stall the program, so when the queue is full it should drop new records and count them rather than wait. Could you change the PR to:
The queue limit can stay as it is. |
29c4b17 to
efb3dc3
Compare
Follow the capture policy requested in boykopovar#10753 and boykopovar#10760. Keep queue callers running, record loss markers and totals, and disable failed output without relying on stream exceptions. AI-assisted: Codex
lgqdaxia
changed the title
|
@oneandonlydean Updated in The queue now drops new records and counts them. When the writer resumes, the trace gets a Write and flush errors now stop tracing and log one error instead of terminating the process. I changed this to direct stream-state checks, so it does not need the exception path that had limited the earlier Windows tests. Both failures now pass with the host runtime and the project's own Windows libc, including later calls doing no more output and normal destruction. Full Windows library/relinker builds and 18 selected checks pass. Linux actual-header host components pass 14/14, plus a real AI-assisted: Codex |
|
@lgqdaxia Thanks for the drop-and-count rework. CI on The four The branch also conflicts with |
efb3dc3 to
48f7852
Compare
Follow the capture policy requested in boykopovar#10753 and boykopovar#10760. Keep queue callers running, record loss markers and totals, and disable failed output without relying on stream exceptions. AI-assisted: Codex
|
@oneandonlydean Thanks, you're right. The fixtures ran locally, but I had missed running the registration audit. Fixed in I reproduced the two registration errors before the change. After reconfiguring, the full Windows build's registration audit reports 1383 test executables, 0 unregistered. Rebuilt the three capture targets and their dependencies; all 16 capture CTests pass, including all four host/project-runtime write/flush cases. On WSL Linux, the actual source and wrapper also pass both host failure tests with a spaced output path; this is component coverage, not a full Linux product build. Rebased onto main AI-assisted: Codex |
|
@oneandonlydean Following up on Build 40458520694: Linux and Windows both passed the registration audit, completed the product compilation and Both jobs failed the same public-header declaration audit, leaving 64 dependent checks per platform not run. The same failure reproduces on the unmodified tested main #11718 has since fixed that duplicate declaration. The source-only audit passes on main If another run is not already queued, could you start a new full CI run on this head against the fixed main when capacity allows? AI-assisted: Codex |
Wait for the writer to take a batch instead of throwing when the pending queue is full. Add a controlled-sink test with four producers and check that records stay complete and ordered. AI-assisted: yes
Remove the extra blank line between capture trace output and frame capture. The independent changes in boykopovar#11157 may merge in either order; the second branch keeps both entries when rebasing. AI-assisted: Codex
Follow the capture policy requested in boykopovar#10753 and boykopovar#10760. Keep queue callers running, record loss markers and totals, and disable failed output without relying on stream exceptions. AI-assisted: Codex
48f7852 to
c4ad483
Compare
|
@oneandonlydean Rebased onto main The manual conflicts were only in the debt file. Both the updated Linux frame-capture entry and our trace-output entry are retained. The capture header, C++ fixtures and failure wrapper are unchanged from On the rebased Windows project configuration, the registration audit passes; all three capture targets build, and 16 capture tests plus the public-header audit pass (17/17). The public-header audit took 60.78 s locally. A separate CPU-only host comparison passes the same eight common tests on main and the change. These checks do not establish a full product build, GPU/game validation or a new hosted CI pass. I've updated the PR's validation summary and kept the previous hosted results clearly marked as historical. If a run on this new head is not already queued, could you start a new full CI run when capacity allows? AI-assisted: Codex |
What
Capture tracing can currently throw when its 16 MiB pending queue fills, or stop the process when file output fails. This follows the capture policy requested in #10753 and replaces this PR's earlier waiting proposal.
capture-trace-dropped records=N; normal trace shutdown writescapture-trace-summary dropped=N. Accepted records and loss markers keep their sequence order. After a drop, records are rejected until the next drain so a shorter record cannot appear before the loss marker.AI-assisted: Codex
#11157 has merged. This rebase preserves its Linux frame-capture debt text, with
Capture trace outputimmediately beforeFrame capture; the remaining upstream debt text is unchanged. PR #7622 also touches the writer-startup lines in the same header. Its independentStartHostThreadchange must be retained alongside this PR'sactiveinitialization and non-throwing stream policy if the branches are later combined. That combination has not been runtime-tested. No GPU semantics are changed.Tested
801d5b9bc11049e6274daca09d70690bd5b8d0a9; new headc4ad483347359e4df0951220d4ca6394a052bc47. Manual conflicts were confined todocs/dev/debt/libSceAgcDriver.md. Removing this PR's single trace-output entry reproduces that base's debt file byte-for-byte. The capture header, both C++ fixtures and the CMake failure wrapper are byte-identical to the previously published head48f785262b5d8e388ed0be86fc954a1c2c1c9dde.BUILD_TESTING=ON.tools/check_test_registration.pypasses, reporting1466 test executable(s), 0 built and not run as recorded, none new. Built the three capture-trace targets and their dependencies, including the project'slibc. All 16 capture-trace CTests pluspublic_headerspass (17/17), including host and project-runtime write/flush failure cases. Total CTest time was 62.80 s; the existingpublic_headersaudit accounted for 60.78 s locally. This was a targeted build and test run, not a full default product build or full CTest suite.ANYPS5_REQUIRE_VULKAN=1,ANYPS5_NO_SHADER_CACHE=1andVK_INSTANCE_LAYERS=VK_LAYER_KHRONOS_validation; their logs contain neither the missing-layer warning norVUID-lines. The fixtures do not initialize Vulkan, so no GPU or validation-layer execution is claimed. Conventions andgit diff --checkpass.48f785262b5d8e388ed0be86fc954a1c2c1c9ddemerged into main32906a0659a5caaea09340b7424091184f4c4293. Both Linux and Windows completed compilation andlibs, and passed their 16 capture-trace tests. Both full CTest jobs failed the same pre-existingpublic_headersaudit, leaving 64 dependent checks per platform not run; #11718 subsequently fixed the duplicate declaration. Those results do not establish hosted CI success for this rebased head. A full CI pass on the new head remains unconfirmed.Checklist
main; no other open PR implements the same functionsNotImplemented_nid_no_patch); silent stubs are listed under Silent stubs in their library's technical debt file.mdfiles or images added to the repository (attach them to this PR)