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

Cache metamethods, finalize references #1

Merged
merged 2 commits into from
Nov 27, 2015

Conversation

RoosterDragon
Copy link
Member

This improves performance in two ways:

  • We cache the reflection needed when making calls involving a LuaCustomClrObject. If a caller reuses the same object in multiple calls, they now only invoke the reflection overhead on the first call. A call involving any LuaTransparentClrObject benefits even more as we can cache this once for the whole app-domain.
  • We add a missing GC.SuppressFinalize call to LuaReference. Callers that take care to dispose these properly can now avoid finalizing these objects.

Fixes OpenRA/OpenRA#9142

This allows us to speed up LuaRuntime.PushCustomClrObject when pushing an object multiple times since we can do this costly reflection on construction on the object and retain it thereafter.

Calling code that uses LuaTransparentClrObject benefits since we can cache this information for the proxy type. It can also benefit calling code that is able to reuse a LuaCustomClrObject instance across multiple Lua calls.
This missing call meant even if the caller took care to dispose a LuaReference it was still being finalized. Adding this call prevents this needless finalization from taking place.
@penev92
Copy link
Member

penev92 commented Nov 13, 2015

FWIW changes seem reasonable to me.

@chrisforbes
Copy link
Member

What's the plan for getting these changes upstream?

@RoosterDragon
Copy link
Member Author

I'm happy to submit this PR upstream too. Figured I'd do one review committee at a time though :)

Having thought about it a bit, I'm wondering if an alternate caching strategy may be better. For example we could cache all this reflection inside the LuaRuntime (as a dictionary lookup on the type). That would remove all the ongoing reflection overhead in OpenRA for scripting. On the other hand, such a cache represents a memory sink if you throw enough types at it. Not a problem for an application that's willing to accept it, but a bit lame in library code.

@RoosterDragon
Copy link
Member Author

Fixes OpenRA/OpenRA#9142, by speeding up the Lua calls for Damaged and Killed.

@chrisforbes
Copy link
Member

👍
On Nov 27, 2015 10:29 AM, "RoosterDragon" notifications@github.com wrote:

Fixes OpenRA/OpenRA#9142 OpenRA/OpenRA#9142,
by speeding up the Lua calls for Damaged and Killed.


Reply to this email directly or view it on GitHub
#1 (comment).

@Mailaender
Copy link
Member

@Mailaender
Copy link
Member

Can't spot regressions in-game. ✅

Mailaender added a commit that referenced this pull request Nov 27, 2015
Cache metamethods, finalize references
@Mailaender Mailaender merged commit 2630107 into OpenRA:openra Nov 27, 2015
@RoosterDragon RoosterDragon deleted the cache-metamethods branch November 27, 2015 14:16
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.

4 participants