Skip to content

Conversation

@Jamiras
Copy link
Member

@Jamiras Jamiras commented Nov 16, 2025

When trying to match an AddSource chain to an existing modified_memref chain, the logic was traversing up the chain to match the root. And it would do this for each subchain as it went.

For example: given the following AddSource chain:

AddSource Mem Byte 0x1230
AddSource Mem Byte 0x1231
AddSource Mem Byte 0x1232
AddSource Mem Byte 0x1233
          Mem Byte 0x1234 = 5

The first Byte 0x1230 would be allocated as a standard memref.

The next Byte 0x1231 would be allocated as a standard memref, and a modified_memref would be created to combine them.

The third Byte 0x1232 is also allocated as a standard memref, and then it looks at the existing modified_memrefs to see if one already exists before creating a new modified_memref that adds the first modified_memref and the new byte.

For the fourth Byte 0x1233, there's two modified_memrefs to examine looking for a match. Neither do, but when checking the second, it walks up the chain 1232->1231->1230 before deciding that it's a different chain than the 1233->1232->1231->1230 it's looking for. This is due to a flaw in the recursion where it has to match the parents of the parents in order to say the parents are the same.

For something like an AddSource chain of 500 items, the code was walking up N log N parents (499+498+497+496...) to not find a match.

Luckily, this only happens when parsing the logic, but it's still unfortunate. With a debug build of PCSX2, loading the FFX subset was taking close to a minute. I'm sure the release build is faster, but would still be more than a few seconds. With these changes, the subset loads almost instantaneously.

I've made two changes in this PR to improve this behavior.

  1. Each node now keeps track of how long its parent chain is, and if the chains have different lengths, it doesn't try to match the parents.
  2. The modifier is checked before the parent chain. If two chains are identical, but one is adding 5 and the other is adding 6, there's no need to check the chains.

@Jamiras Jamiras added this to the 12.2.0 milestone Nov 16, 2025
@wescopeland
Copy link
Member

That memory leak is mind boggling, but I've verified the actual logic changes locally with a benchmark script to parse FFX's triggers:

     Benchmark 1: develop
       Time (mean ± σ):     14.714 s ±  0.046 s    [User: 14.708 s, System: 0.004 s]
       Range (min … max):   14.633 s … 14.792 s    10 runs

     Benchmark 2: PR #482
       Time (mean ± σ):     158.3 ms ±   2.1 ms    [User: 156.5 ms, System: 2.1 ms]
       Range (min … max):   155.9 ms … 161.1 ms    10 runs

     Summary
       PR #482 ran
        92.94 ± 1.25 times faster than develop

@Jamiras Jamiras merged commit 84f3f9b into RetroAchievements:develop Dec 1, 2025
6 checks passed
@Jamiras Jamiras deleted the bugfix/chain_dedup_perf branch December 1, 2025 14:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants