Skip to content

perf(agc): count hook calls only while profiling - #858

Closed
meh7an wants to merge 1 commit into
boykopovar:mainfrom
meh7an:perf/agc-hook-counters-when-profiling
Closed

meh7an wants to merge 1 commit into
boykopovar:mainfrom
meh7an:perf/agc-hook-counters-when-profiling

Conversation

@meh7an

@meh7an meh7an commented Oct 6, 2026

Copy link
Copy Markdown
Contributor

What

FlushForAccess incremented two process-wide atomics, hookCalls and hookLocks, on every guest memory access it handled. They're only printed by the [recorder] report under APS5_PROFILE_DRAW, so they're now counted only then.

Tested

Windows 11, MinGW GCC 15.2, RTX 2080 Ti. This branch builds, and the 165 agc driver, recompiler and execution test targets pass with it stacked on the other changes I'm sending (#849 and the perf PRs opened with this one). I haven't measured this change alone: together with 14 changes of the same kind on my working branch it took Sonic Origins Plus's heaviest menu page (about 1,650 draws per frame) from 48.0 to 43.6 ms per frame (20.8 to 22.9 FPS).

Checklist

  • Based on current main; no other open PR implements the same functions
  • One topic per PR; follow-ups go in a new PR
  • No comments in code except technical debt
  • Unimplemented paths throw (NotImplemented_nid_no_patch); silent stubs are listed in TechnicalDebt
  • No notes, investigation .md files or images added to the repository (attach them to this PR)
  • New third-party code is a submodule built from source
  • Generic behaviour, not specific to one title
  • Depends on: none
  • AI-assisted: yes

@github-actions github-actions Bot added the agc libSceAgc* label Oct 6, 2026
@oneandonlydean

Copy link
Copy Markdown
Collaborator

Thanks. hookCalls and hookLocks are only read by the [recorder] report under APS5_PROFILE_DRAW, and DrawProfiled() is a cached static, so this doesn't change anything outside profiling.

One thing before it can go in. Performance PRs need before/after numbers for the change itself (the owner asked for this on #218). The 48.0 to 43.6 ms figure is for 15 changes together. Could you measure main against main plus this PR alone on the same page? The mean frame time over a few runs is enough, or the FlushForAccess share of the profile.

@Dextroid17 Dextroid17 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Built and ran it — external PRs get no CI here.

Linux (CachyOS, GCC, Release), NVIDIA RTX 3080 Ti:

cmake --build build --parallel 16
ctest --test-dir build -j 16    →  359/359 passed

The guard is sound, and I checked both halves of that.

First, that it is genuinely cheaper than what it replaces. DrawProfiled() is a function-local static:

bool DrawProfiled() {
    static const bool profiled = std::getenv("APS5_PROFILE_DRAW") != nullptr;
    return profiled;
}

So it is one getenv on the first call and a plain load of an initialised static afterwards — against a fetch_add(relaxed), which is a locked read-modify-write and the more expensive of the two on an uncontended cache line. Trading the locked RMW for the load is the real win, and it survives the fact that this runs per hook call. If it had been a getenv per call the change would have been a pessimisation, so it was worth confirming rather than reading the name.

Second, that gating the increment cannot change what anyone observes. hookCalls and hookLocks are read in exactly one place — the APS5_PROFILE_DRAW report at the end of Recorder.cpp (hook %llu calls, %llu locked) — so when profiling is off the counters are never printed, and when it is on both increments still happen as before. There is no third reader that would now see a zero where it previously saw a count.

The static also removes the one case that could have made the numbers misleading: because the flag is captured once, it cannot flip mid-run and leave a partial count.

What I could not do: measure it. The counters are per hook call, so the saving is real per call, but I have no workload here that isolates FlushForAccess, and I would rather report equivalence and no regression than quote a timing I did not take.

This is a comment, not an approval — I have no merge rights here.

Written with AI assistance.

@oneandonlydean

Copy link
Copy Markdown
Collaborator

Same as #857: please reopen with numbers for this change alone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

agc libSceAgc*

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants