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

JIT: reuse trampolines when possible #1218

Merged
merged 1 commit into from Oct 7, 2014

Conversation

hthh
Copy link

@hthh hthh commented Oct 4, 2014

In tests, this showed hundreds of cache hits. A huge number of hits were, to my surprise, write trampolines with the same PC. I'm inferring from this that JIT code may be repeatedly invalidated and re-generated.

It seems this could easily lead to a trampoline-memory-leak causing the "Trampoline cache full" issue: https://code.google.com/p/dolphin-emu/issues/detail?id=7661

However, I was unable to reproduce that issue, so I'm not sure if this fixes it or not.

@phire
Copy link
Member

phire commented Oct 4, 2014

@dolphin-emu-bot rebuild

@@ -11,6 +11,19 @@
// We need at least this many bytes for backpatching.
const int BACKPATCH_SIZE = 5;

struct TrampolineCacheKey {

This comment was marked as off-topic.

@hthh hthh force-pushed the trampolinecaching branch 2 times, most recently from 347b1fa to 265e608 Compare October 4, 2014 01:57
@hthh
Copy link
Author

hthh commented Oct 4, 2014

Oops. I just noticed Jit64::ClearCache calls trampolines.ClearCodeSpace(), which would probably break this quite dangerously...

@hthh
Copy link
Author

hthh commented Oct 4, 2014

I fixed up those problems, and investigated the cause of the excessive back-patching:
https://docs.google.com/document/d/1mJgMcmefJIW8Bx139BcaigmhU89S1vtPPgk1sfzUIi4/edit?usp=sharing

I think this PR is still a good idea (and should fix the trampoline cache full error). Other changes to the JIT should probably be made to address the underlying problem.

edit: wrong link >_>

@skidau
Copy link
Contributor

skidau commented Oct 5, 2014

Thanks for the investigation, @hthh. Since you have found out that it is not a leak would you please also double the trampoline cache size? This will account for the addition of fastmem-writes on top of fastmem-reads. We can merge this PR after that is done. I do find it a bit strange that the issue does not occur on Windows and that it took over a year for it to be reported, considering that SSBB is one of the most popular games on the Wii.

@skidau
Copy link
Contributor

skidau commented Oct 5, 2014

@dolphin-emu-bot rebuild

@FioraAeterna
Copy link
Contributor

I can't read the document you linked; could you make it public?

@FioraAeterna
Copy link
Contributor

Could there be a nicer solution to that FIFO problem -- maybe tracking when an instruction fastmem-faults so that when the block is recompiled, we just disable fastmem for that particular load or store?

@hthh
Copy link
Author

hthh commented Oct 5, 2014

@skidau You're absolutely right, it's a very strange problem, and I don't have any explanation for the Windows/Linux/OS X mysteries. The only thing I'm sure about is that the quadratic rate of trampoline generation is wrong. It's probably worth checking that other people find that this fixes it for them.

It might be worth noting that trampolines have grown in size over time (the PC argument was added, which would make each trampoline a bit larger), which might explain why it only started getting noticed recently.

Anyway, I've doubled the trampoline cache size and hopefully fixed the Windows build warnings.

@FioraAeterna That sounds like a good approach, and would remove the back-patching overhead (in my tests, there are still 20000 redundant segfaults). I'm not sure that would be a complete fix: the bigger a block (of FIFO writes) is, the more times it will be recompiled, which could still cause problems (quadratic compilation time and quadratic code allocations come to mind).

I'd like to keep this code because I think it will, generally speaking, prevent redundant trampoline generation. It's definitely not the most complete or most elegant fix.

@FioraAeterna
Copy link
Contributor

I'm going to write up a patch for this and see how well it works

@FioraAeterna
Copy link
Contributor

#1222

@PatrickFerry
Copy link
Contributor

@hthh that's a very big problem you've discovered.

skidau added a commit that referenced this pull request Oct 7, 2014
JIT: reuse trampolines when possible
@skidau skidau merged commit b3b34d1 into dolphin-emu:master Oct 7, 2014
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
6 participants