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

Reset map cache between games #16944

Merged
merged 1 commit into from Aug 25, 2019

Conversation

@teinarss
Copy link
Contributor

commented Aug 13, 2019

Should help a bit with memory issue in #16515

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 14, 2019

Summarising my thoughts from IRC: I'm worried that things may go unexpectedly wonky if someone resets the map cache while still holding on to MapPreviews somewhere. My preference is to dispose and replace the whole ModData at the top level of the dedicated server level, which will be much cleaner provided we can work out how to fix the remaining issues there.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 14, 2019

@pchote pchote added this to the Next Release milestone Aug 14, 2019

@teinarss teinarss force-pushed the teinarss:reset_mapcache branch from ac7f145 to ee9c7f3 Aug 19, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 19, 2019

Updated

@@ -939,5 +939,10 @@ public static void FinishBenchmark()
Exit();
}
}

public static void Clean()

This comment has been minimized.

Copy link
@pchote

pchote Aug 19, 2019

Member

Can we call this ClearDelayedActions?

This comment has been minimized.

Copy link
@pchote

pchote Aug 19, 2019

Member

This actually raises a bigger issue: why is the server queuing delayed actions in the first place, knowing that will never be run (LogicTick isn't called)?

An even simpler fix could be to add an if-not-server check to the thing in the map cache that is queueing these useless tasks. RunAfterTick and RunAfterDelay could check the same thing and throw an exception if the server tries to use them.

This comment has been minimized.

Copy link
@pchote

pchote Aug 20, 2019

Member

Something like 7bbaa43 to consume the events immediately would IMO be the best compromise between simplicity/cleanness.

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 20, 2019

@pchote

This comment has been minimized.

Copy link
Member

commented Aug 20, 2019

Good point - add an if (Dedicated) around the server.cs change.

@teinarss teinarss force-pushed the teinarss:reset_mapcache branch from ee9c7f3 to 5a9ea2d Aug 22, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 22, 2019

Updated

@pchote
Copy link
Member

left a comment

LGTM after adding the comment

@teinarss teinarss force-pushed the teinarss:reset_mapcache branch from 5a9ea2d to 14d368f Aug 24, 2019

@teinarss

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2019

Updated

@pchote
pchote approved these changes Aug 24, 2019

@abcdefg30 abcdefg30 merged commit 4dd8472 into OpenRA:bleed Aug 25, 2019

2 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@abcdefg30

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.