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

Use virtual memory to allocate g_mem_base (if possible). #468

Closed
wants to merge 1 commit into from

Conversation

bsmiles32
Copy link
Member

This should fix issue #467

Basically, it replaces (if possible) the big malloc with virtual memory allocation in such a way that no unneeded physical pages gets allocated. When available (-DHAVE_MMAP) we use the mmap/munmap/mprotect functions, otherwise if it is a Windows plateform we use the VirtualAlloc/VirtualFree functions and otherwise only rely on the C standard library malloc/free and hope that internally, given the size of the alloc, it will use virtual memory.

I have tested myself on Linux with and without mmap, but couldn't test other platforms.

@fzurita
Copy link
Member

fzurita commented Nov 4, 2017

This didn't work. Allocation succeeds, but the core crashes when trying to use the memory allocated. I'll try to get a back trace later today. It looks like Samsung is not implementing mmap correctly in their kernel.

@fzurita
Copy link
Member

fzurita commented Nov 4, 2017

Ok, it seems that API level less than 21 doesn't support mmap... That's terrible. API level 21 is Android 5.0.

How hard would it be to implement a fallback mechanism to the old memory allocation method?

@fzurita
Copy link
Member

fzurita commented Nov 5, 2017

Ok, I must had been looking at the wrong log file. It didn't crash, but the allocation failed. We got the M64ERR_NO_MEMORY error code. I made sure that the mmap portion of the code was being used by removing the malloc and Windows versions.

I think it's the mmap called that failed because I never saw "Failed to change memory protection."

@bsmiles32
Copy link
Member Author

bsmiles32 commented Nov 5, 2017

Rebased against master, fixed the C99-ism (variable declaration inside for loop).

@fzurita Does the workaround suggested in [1] works ? (eg reducing dalvik.vm.heapsize to increase chances of having a continuous 512M chunk available ?).

Ok, it seems that API level less than 21 doesn't support mmap... That's terrible. API level 21 is Android 5.0

Do you have a link for that please ?

[1] https://stackoverflow.com/questions/33897711/android-mmap-fails-with-out-of-memory

@fzurita
Copy link
Member

fzurita commented Nov 5, 2017

This is what I found suggesting that:

android/ndk#449

It may be that it only affects mmap64.

I did not find the page you found. That's a nice find. I'll look into that.

@bsmiles32
Copy link
Member Author

android/ndk#449

In this link they use mmap for file-backed memory, not anonymous allocation. So I think the issue was more that in their device your users didn't have a continuous 512M block available in their virtual address space. Trying the suggested workaround might help (and maybe even without this PR).

@fzurita
Copy link
Member

fzurita commented Nov 5, 2017

@bsmiles32 The proposed solution requires a rooted device. It's not something I can change for the app. It's something users have to do on their devices.

@fzurita
Copy link
Member

fzurita commented Nov 6, 2017

Currently, about 25% of the users of Mupen64plus-AE on Android are using 5.0 to 5.1.

They are probably all affected. Since there is nothing I can do from the app itself, it's probably better to have some kind of workaround.

One option would be for Mupen64plus-AE to roll back to the older version of the core.

Another option would be to keep two versions of the core, an old one and a newer up to date one. In the app, if I detect Android 5.0 or 5.1, I can make the app use the older version of the core.

@loganmc10
Copy link
Member

I thought most Linux systems used mmap under the hood anyway. If malloc is returning NULL, I don't think you're going to have much luck tricking it into succeeding. Android has a mind of its own when it comes to memory management.

From looking at the code, the only real benefit g_mem_base currently offers is getting rid of the if statement in fast_mem_access, but I believe the plan was to implement this: #277, which would make having some kind of fallback system probably quite complex.

Anyway if #277 isn't going to happen, then I'd say get rid of g_mem_base, but if that is going to happen, this is quite a tricky problem...

@fzurita
Copy link
Member

fzurita commented Nov 6, 2017

What kind of speed up would #277 give?

If it's a 10% speed up, it wouldn't be of much help since the dynarec uses very little compared to the graphics plugin.

@bsmiles32
Copy link
Member Author

I will push another attempt when I get back home tonight, which tries the big 256M alloc first and if it fails does a smaller alloc (~73M) and uses some if/else if/else for selecting the right address.

In my wip proof of concept of fast mem I wasn't able to get much speed up for the pure/cached interpreter because of the overhead of page faults (< 2% speedup if I remember correctly). However, for the dynarec we can optimistically emit the page-fault way and if it triggers too much regen the block with the non page-fault way, I expect maybe a 10% speedup, but as you say this may not be the bottleneck, so meh. Anyway, this work is still very early WIP and may not be successfull after all.

@bsmiles32
Copy link
Member Author

Replaced with another approach in #474

@bsmiles32 bsmiles32 closed this Nov 8, 2017
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.

3 participants