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

MemArena: Use memory placeholders for fastmem on Windows to ensure nothing allocates within the fastmem space. #9544

Merged

Conversation

AdmiralCurtiss
Copy link
Contributor

@AdmiralCurtiss AdmiralCurtiss commented Mar 1, 2021

This allocates and handles our fastmem address region as a placeholder on Windows. This avoids the possibility that something else allocates memory within the fastmem address region after it has been set up.

This should fix the bug (on Windows at least) where large texture packs break emulation, such as 12404 or 11994, but I cannot 100% confirm it.

  • Use old logic as fallback if VirtualAlloc2/MapViewOfFile3 are unavailable (as they would be on eg. Win7).
  • Make sure we load the VirtualAlloc2/MapViewOfFile3 from the correct dll. (see Windows API sets; maybe this one...?)
  • Memory cleanup on shutdown.
  • Error handling.
  • 32bit support.

References:
Test code
Docs on VirtualAlloc2
Docs on VirtualFree
Docs on MapViewOfFile3
Docs on UnmapViewOfFileEx

@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-placeholder-windows branch 4 times, most recently from 5f522e4 to d20a699 Compare March 7, 2021 02:43
@AdmiralCurtiss AdmiralCurtiss marked this pull request as ready for review March 7, 2021 02:57
@AdmiralCurtiss
Copy link
Contributor Author

Alright, I think I'm at a point where I want some external input on this. Lemme have it.

And for reference, yes, this does appear to fix at least one variant of the large texture pack issue. Though it's hard to say exactly due to the sporadic nature of it.

@AdmiralCurtiss AdmiralCurtiss changed the title [WIP] Use placeholder for fastmem on Windows to ensure nothing allocates within the fastmem space. Core/Memmap: Use placeholder for fastmem on Windows to ensure nothing allocates within the fastmem space. Mar 7, 2021
Copy link

@Warepire Warepire left a comment

Choose a reason for hiding this comment

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

Perhaps something for later, it doesn't seem like many class methods with #ifdef in them share code, would it make sense to split the class into OS-specific implementations rather than having everything in the same implementation?

Source/Core/Common/MemArena.cpp Outdated Show resolved Hide resolved
Source/Core/Common/MemArena.cpp Outdated Show resolved Hide resolved
Source/Core/Common/MemArena.cpp Outdated Show resolved Hide resolved
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-placeholder-windows branch 3 times, most recently from f5a13f3 to 4a70d11 Compare March 7, 2021 22:37
@AdmiralCurtiss
Copy link
Contributor Author

Okay, I've updated this:

  • The MemArena has been split up into three separate files for each OS, which is indeed way cleaner.
  • The Windows MemArena now handles the splitting/joining stuff internally, so you can just map and unmap wherever without any setup (as long as those regions are free, of course).

As far as I'm concerned, this is done now, but it still needs testing on an old Windows system that does not have VirtualAlloc2 and MapViewOfFile3. A Linux and Android implementation of a similar thing would also be good, but I'd leave that to a followup PR if that's okay.

Source/Core/Common/MemArenaAndroid.cpp Outdated Show resolved Hide resolved
Source/Core/Common/MemArenaUnix.cpp Outdated Show resolved Hide resolved
Source/Core/Common/MemArenaWin.cpp Outdated Show resolved Hide resolved
@AdmiralCurtiss AdmiralCurtiss changed the title Core/Memmap: Use placeholder for fastmem on Windows to ensure nothing allocates within the fastmem space. MemArena: Use memory placeholders for fastmem on Windows to ensure nothing allocates within the fastmem space. Jul 24, 2021
@PatrickFerry
Copy link
Contributor

PatrickFerry commented Nov 27, 2021

I've asked a few people with the issue with texture packs to use this build and every one of them has had success in resolving the issue. They were all modern windows users though.

EDIT: I'll check if this works with Windows 7

@Pokechu22
Copy link
Contributor

Apart from my comments above, the first two commits look good. I don't know enough about the Windows API to say that the 3rd commit is good, but nothing seems wrong with it either.

@AdmiralCurtiss
Copy link
Contributor Author

Appreciate it.

@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-placeholder-windows branch 2 times, most recently from 754ef77 to 84fe882 Compare December 18, 2021 23:53
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-placeholder-windows branch 2 times, most recently from b622417 to 5389cfb Compare December 30, 2021 00:07
@AdmiralCurtiss AdmiralCurtiss force-pushed the fastmem-placeholder-windows branch 2 times, most recently from 5c6c2aa to 0586b2f Compare January 31, 2022 02:41
@JMC47
Copy link
Contributor

JMC47 commented Feb 9, 2022

This has been sitting for a very long time and no one else has reviewed it. Some of the most popular modern texture packs, like the Wind Waker Hytapia one. I continue to see people on the unofficial discord running into this crash, so I think it's about time that we merged it. I'll leave this for a few more hours to give people a chance, but if no one speaks up that they plan on reviewing it, I'll just merge.

Performance shouldn't be lower, and games seem to crash less.

@JMC47 JMC47 merged commit a6f9dd5 into dolphin-emu:master Feb 9, 2022
10 checks passed
@AdmiralCurtiss AdmiralCurtiss deleted the fastmem-placeholder-windows branch February 9, 2022 21:47
@AdmiralCurtiss
Copy link
Contributor Author

Much appreciated.

@AhayriSG
Copy link

ck if this works with Window

"I'll check if this works with Windows 7"

Minimum supported client | Windows 8 (https://docs.microsoft.com/en-us/windows/win32/api/memoryapi/nf-memoryapi-unmapviewoffileex)

image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
7 participants