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

Don't add segfault handler in interpreter mode #328

Merged
merged 3 commits into from Jul 6, 2014

Conversation

Tilka
Copy link
Member

@Tilka Tilka commented Apr 30, 2014

...and use enum constants instead of plain numbers for referring to CPU backends.

@Tilka
Copy link
Member Author

Tilka commented Apr 30, 2014

(Fixed build on non-x86.)

@Tilka
Copy link
Member Author

Tilka commented Apr 30, 2014

Is there a nice way to fix the warning about "packet >> iCPUCore"?

(This is a serious problem, so don't merge this PR yet.)

@shuffle2
Copy link
Contributor

@dolphin-emu-bot rebuild

@shuffle2
Copy link
Contributor

@Tilka I'm not sure of a good way. Maybe specify the underlying type of the enum? Worst case, you could call the correct sf::Packet::operator>>(whatever) explicitly.

@Sonicadvance1
Copy link
Contributor

Instead of having the startupparameter value for the CPU core be a enum object, couldn't we just have it be a u8 and compare it to the enums instead?
Kind of a bad programming practice though.
What if the enum was strongly typed? Might cause other issues though.

@shuffle2
Copy link
Contributor

shuffle2 commented Jun 7, 2014

Oh, you can also specialize the operator implementation, which is probably the "proper" way to do it, although I'm not a huge fan. Something like http://stackoverflow.com/questions/1297609/overloading-friend-operator-for-template-class

@@ -82,6 +82,15 @@ enum Hotkey
NUM_HOTKEYS,
};

enum CPUBackend

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@Tilka
Copy link
Member Author

Tilka commented Jun 13, 2014

Changed to enum CPUBackend : u8 and rebased to current master.

@Tilka
Copy link
Member Author

Tilka commented Jun 14, 2014

Ready to merge from my side.

@@ -83,6 +83,15 @@ enum Hotkey
NUM_HOTKEYS,
};

enum CPUBackend : u8

This comment was marked as off-topic.

This comment was marked as off-topic.

This comment was marked as off-topic.

@neobrain
Copy link
Member

Apart from the things I pointed out, LGTM.

@Tilka
Copy link
Member Author

Tilka commented Jul 5, 2014

Resolved merge conflict and addressed all remaining comments.

@neobrain
Copy link
Member

neobrain commented Jul 6, 2014

@Tilka: This comment grants you the permission to merge this pull request whenever you think it is ready. After addressing the remaining comments, click this link to merge.


@dolphin-emu-bot allowmerge

dolphin-emu-bot added a commit that referenced this pull request Jul 6, 2014
Don't add segfault handler in interpreter mode
@dolphin-emu-bot dolphin-emu-bot merged commit 4ec8c37 into dolphin-emu:master Jul 6, 2014
@Tilka Tilka deleted the enum_cpubackend branch July 6, 2014 17:29
@Linktothepast
Copy link
Contributor

I am getting a 10 to 20 % performance regression overall after this merge (tried DKC returns and Super Mario galaxy, DKC went from 72 fps to 55 fps in the overall map for example).

@neobrain
Copy link
Member

neobrain commented Jul 6, 2014

Are you sure about this? As far as I can see, this branch doesn't change the behavior of JIT code at all.

@Linktothepast
Copy link
Contributor

Yes i am pretty sure about this. Also it has another issue too, if i change the cpu engine, close and restart dolphin the cpu core will always be set to interpreter.

@Linktothepast
Copy link
Contributor

Perhaps it uses jitil instead of jit or something? That might be an explanation of the performance regression.

@Tilka
Copy link
Member Author

Tilka commented Jul 6, 2014

@Linktothepast Can't reproduce the config issue. Can you be more detailed? Did you start any games in-between?

@Linktothepast
Copy link
Contributor

It is quite easy to reproduce, change the cpu engine to jitil and close dolphin (i didn't need to boot any games and by closing i mean the program altogether). Start dolphin again and see the default choice being set to interpreter instead.

@Tilka
Copy link
Member Author

Tilka commented Jul 7, 2014

Never mind, I see the problem. (It's quite embarrassing lol.)

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