Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jit64 / JitArm64: Optimized idle skipping detection. #7287

Merged
merged 4 commits into from Apr 22, 2019

Conversation

degasus
Copy link
Member

@degasus degasus commented Jul 28, 2018

This patch tries to detect idle loops instead of hardcoding them. Half of the patch is from @delroth.

But it lacks MMIO detection: We must not assume that a load has no side effect on hitting MMIO.

@JMC47
Copy link
Contributor

JMC47 commented Jul 28, 2018

Xenoblade Chronicles sees a huge boost in performance (up to 2x as fast in lighter scenes)

Fire Emblem Radiant Dawn is 6% slower, Luigi's Mansion appears to be a bit slower in lightweight scenes as well (down from 1600 fps in the main menu to 1200 FPS on my computer, rough estimate.)

Copy link
Member

@delroth delroth left a comment

Choose a reason for hiding this comment

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

<3<3<3 seeing my idle skipping experiment be resurrected. Thanks!

Source/Core/Core/PowerPC/PPCAnalyst.cpp Show resolved Hide resolved
Source/Core/Core/PowerPC/PPCAnalyst.cpp Show resolved Hide resolved
@degasus degasus changed the title WIP: Jit64: Optimized idle skipping detection. WIP: Jit64 / JitArm64: Optimized idle skipping detection. Jul 28, 2018
@degasus degasus force-pushed the idle_skipping branch 2 times, most recently from a130b9a to 0f26f27 Compare July 28, 2018 14:48
@JMC47
Copy link
Contributor

JMC47 commented Jul 28, 2018

Fire Emblem: Radiant Dawn is still a bit slower in this Pull Request, ~180 to 168 vs master (was 160 before the recent change) in the Wii Remote Strap Screen and carries a similar framerate loss throughout the FMVs/menus.

Bully: Scholarship Edition runs 22 fps instead of 20 on the Wii Remote screen but is ~1% slower in-game, probably within margin of error.

Xenoblade is really fast though.

@Miksel12
Copy link
Contributor

Has anyone tested the performance after the commit from 9 aug? If the overall performance is better, I think it would be a worthwhile change.

@degasus
Copy link
Member Author

degasus commented Feb 23, 2019

It is a huge speedup for ~3 games. It is very worth it, I was just to lazy to finish it.

@Miksel12
Copy link
Contributor

Is there in the current state still performance regressions for certain games?

@weihuoya
Copy link
Contributor

yes, Fire Emblem Radiant Dawn is 6% slower.
but speedup many game.

@Narugakuruga
Copy link

Slow down Monster Hunter Tri from around 400 fps to around 390 fps in menu.

@degasus degasus force-pushed the idle_skipping branch 4 times, most recently from 610aee1 to 13e6a5e Compare April 16, 2019 06:45
@degasus degasus changed the title WIP: Jit64 / JitArm64: Optimized idle skipping detection. Jit64 / JitArm64: Optimized idle skipping detection. Apr 16, 2019
@zlice
Copy link

zlice commented Apr 17, 2019

Need For Speed: Hot Pursuit 2 still lags with this PR and #7630 - but with master as of Apr 16 2019 (5c5e6df) just removing the OPCD 32 (LWZ) skip makes it run fine.

Both this and 7630 allow RE0 to use the manhole opener in the train

@JMC47
Copy link
Contributor

JMC47 commented Apr 17, 2019

What do you mean still lags with this PR? Is there some kind of bug causing it to lag? Or is it just slower?

@JosJuice
Copy link
Member

JosJuice commented Apr 17, 2019

When on dual core, disabling idle skipping or just SyncOnSkipIdle increases the performance in Need for Speed: Hot Pursuit 2 (and sometimes also causes severe FPS/VPS desyncs, from the little I tested). Disabling idle skipping on single core just decreases the performance with no other apparent effect, as expected. So I don't think there's any bug with that game and idle skipping.

@JMC47
Copy link
Contributor

JMC47 commented Apr 17, 2019

Ah, yeah, it runs completely unsynced and has a chance of crashing. Which is what I wrote in the progress report.

@zlice
Copy link

zlice commented Apr 17, 2019

Single core and SyncOnSkipIdle = False, NFS:HS2 is still is unplayable.

The main point I was making - is with master just removing the LWZ chunk in Jit_LoadStore.cpp allows it to run completely fine for me.

This PR removes that chunk, but it still runs like crap.

JMC47 - I'd be interested to know if the same is true for Xenoblade Chronicles. And if Fire Emblem RD has any hit.

@JosJuice
Copy link
Member

Single core and SyncOnSkipIdle = False, NFS:HS2 is still is unplayable.

Yes, SyncOnSkipIdle has no effect on single core. If you want the fast but unstable behavior, you need to use dual core with SyncOnSkipIdle = False. This is not a bug and will not be addressed by this PR.

@degasus
Copy link
Member Author

degasus commented Apr 17, 2019

@zlice This is the expected behavior on NFS. This is a variable FPS game, so you need SyncGPU. IdleSkipping is just a bad hack around it.

@zlice
Copy link

zlice commented Apr 17, 2019

git 5c5e6df
!OPCD32Skip && dualCore == NFS_SUCCESS

git 5c5e6df
git merge #7287
!OPCD32Skip && extraCode && dualCore == NFS_SUCKS

I don't know how else to put this. "I've tried every combo" ? "Will someone else verify" ? What am I saying that is not coming across?

@JosJuice
Copy link
Member

JosJuice commented Apr 17, 2019

Your description of how this PR behaves is correct – the disagreement is that we don't think that behavior is a problem.

This PR does two things: It removes the old idle skipping code (which your comment calls "OPCD32Skip"), and adds new idle skipping code in new places (which your comment calls "extraCode"). In order to not have any idle skipping, neither of those two must be present. But there is no good reason to remove idle skipping entirely, because you can get the effect that you want by disabling SyncOnSkipIdle instead.

Please stop commenting about Need for Speed: Hot Pursuit 2. It's derailing the discussion of this PR.

@zlice
Copy link

zlice commented Apr 17, 2019

OK, I /think/ I know where everyone is coming from with SyncOnSkipIdle and IdleSkip relation.

I believe the slow down may be caused from WriteExceptionExit(). MerryMage said it "does not flush 'anything' " (not entirely sure what that means).

Not sure if that's too far off from this PR, or helpful, or just the way that function is.

Can test WriteExceptionExit() calls later if it helps.

@JosJuice
Copy link
Member

I believe the slow down may be caused from WriteExceptionExit(). MerryMage said it "does not flush 'anything' " (not entirely sure what that means).

The problem with WriteExceptionExit that MerryMage explained is what leads to freezes in Resident Evil 0 and CSI: Hard Evidence in master, but it does not affect the performance in any meaningful way.

Copy link
Contributor

@merryhime merryhime left a comment

Choose a reason for hiding this comment

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

In principle, looks good to me.

Need to perhaps consider what happens when debugging though.

Source/Core/Core/PowerPC/PPCAnalyst.cpp Show resolved Hide resolved
Source/Core/Core/PowerPC/PPCAnalyst.cpp Show resolved Hide resolved
@JosJuice
Copy link
Member

With the recent changes to the PR, CSI: Hard Evidence works with cached interpreter (in addition to this PR making it work with Jit64 and JitArm64 earlier). However, it seems like regular interpreter freezes during one of the prerendered videos, like before this PR. (It also takes something like 20 minutes to just get to the start of the prerendered videos, compared to 25 seconds when running with JIT at full speed...)

@degasus
Copy link
Member Author

degasus commented Apr 22, 2019

@MerryMage I think the debug helpers are not needed here. Idle is called when the idle loop is fully executed, so both stepping and breakpoints prevent the idle logic here.

@merryhime
Copy link
Contributor

@degasus Fine with me.

@degasus degasus merged commit 2abe333 into dolphin-emu:master Apr 22, 2019
Techjar added a commit to Techjar/dolphin that referenced this pull request Apr 24, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet