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

Some core cleanup #10420

Merged
merged 5 commits into from Dec 20, 2017

Conversation

Projects
None yet
2 participants
@hrydgard
Owner

hrydgard commented Dec 19, 2017

Just moving stuff around and deleting obsolete ifdefs, mostly. Also removed the old "event" sync primitive, replaced it with direct condition variable use. EDIT: Note that all I did was inline the calls, so bugs with the old events are now exposed and should be fixed in a later pull request.

To make some other changes easier.

@@ -95,7 +97,7 @@ void Core_Halt(const char *msg) {
void Core_Stop() {
Core_UpdateState(CORE_POWERDOWN);
Core_NotifyShutdown();
m_hStepEvent.notify_one();
m_StepCond.notify_one();

This comment has been minimized.

@unknownbrackets

unknownbrackets Dec 19, 2017

Collaborator

I think "events" and condvars are slightly different, hopefully not in an important way for us... but, do we need to lock the mutex here? It's possible that this code might run after Core_IsActive() but before m_InactiveCond.wait(guard);...

-[Unknown]

This comment has been minimized.

@hrydgard

hrydgard Dec 20, 2017

Owner

Note that all I did was inline the contents of the event method calls, so nothing changed. As the resulting code does not look right, this exposes that the events are a broken idea :)

Fixing the bugs is out of scope for this pull requests but should be done after.

This comment has been minimized.

@unknownbrackets

unknownbrackets Dec 20, 2017

Collaborator

Hmm, I thought events had used Windows events, and those tracked a flag (which is why .reset() existed?) Did we change at some point?

-[Unknown]

This comment has been minimized.

@hrydgard

hrydgard Dec 20, 2017

Owner

Events have long used condition variables on platforms other than Windows (for the Qt debugger, I guess) and a while ago I ripped out the Windows-ifdeffed path since the condition_variable one appeared to work. And it does, but it's clearly not clean and there might be some rare race conditions.

@@ -112,13 +114,15 @@ bool Core_IsInactive() {
void Core_WaitInactive() {
while (Core_IsActive()) {
m_hInactiveEvent.wait(m_hInactiveMutex);
std::unique_lock<std::mutex> guard(m_hInactiveMutex);
m_InactiveCond.wait(guard);

This comment has been minimized.

@unknownbrackets

unknownbrackets Dec 19, 2017

Collaborator

I guess this should check Core_IsActive() again inside the lock?

-[Unknown]

This comment has been minimized.

@hrydgard

hrydgard Dec 20, 2017

Owner

All the previous calls to event methods should be revisited to look for bugs, yeah :)

@@ -251,7 +255,7 @@ void Core_SingleStep() {
static inline void CoreStateProcessed() {
if (coreStatePending) {
coreStatePending = false;
m_hInactiveEvent.notify_one();
m_InactiveCond.notify_one();

This comment has been minimized.

@unknownbrackets

unknownbrackets Dec 19, 2017

Collaborator

Maybe we need a lock here too?

-[Unknown]

#if defined(_DEBUG)
// Many platforms, like Android, do not call this function but handle things on their own.
// Instead they simply call NativeRender and NativeUpdate directly.
void Core_Run(GraphicsContext *ctx) {

This comment has been minimized.

@unknownbrackets

unknownbrackets Dec 19, 2017

Collaborator

Does Qt not call this function anymore? I think that's why we had all the ifdefs, so its debugger would work, or something...

I guess we no longer use Qt on mobile?

-[Unknown]

This comment has been minimized.

@hrydgard

hrydgard Dec 20, 2017

Owner

It doesn't. And yeah, we no longer support any Qt-on-mobile platform so we could clean up a bit more...

Add missing includes. Remove some more unnecessary #ifdef _DEBUG chec…
…ks - the debugger is supposed to work in release mode too.
@hrydgard

This comment has been minimized.

Owner

hrydgard commented Dec 20, 2017

Squashed the buildfix commits, merging. Will look into the possible condition_variable bugs later, but they aren't new as mentioned.

@hrydgard hrydgard merged commit 2bdae5b into master Dec 20, 2017

0 of 4 checks passed

continuous-integration/appveyor/branch Waiting for AppVeyor build to complete
Details
continuous-integration/appveyor/pr Waiting for AppVeyor build to complete
Details
continuous-integration/travis-ci/pr The Travis CI build is in progress
Details
continuous-integration/travis-ci/push The Travis CI build is in progress
Details

@hrydgard hrydgard deleted the core-cleanup branch Dec 20, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment