Skip to content

Improve TypeManager.function/TypeManager.virtual_function fetch speed by using cache. #479

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.

Sign up for GitHub

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from
+46 −20
Open

Improve TypeManager.function/TypeManager.virtual_function fetch speed by using cache. #479

Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
094c66b
Improve TypeManager.function/TypeManager.virtual_function fetch speed…
CookStar Jun 14, 2023
db804ed
Removed MemberFunction as an attribute.
CookStar Jun 17, 2023
3d5311d
Minor fixes.
CookStar Jun 17, 2023
07e7bba
Changed virtual_function to use a function cache that already exists.
CookStar Jun 21, 2023
60808ea
Removed unnecessary _ptr call.
CookStar Jun 25, 2023
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
81 changes: 54 additions & 27 deletions addons/source-python/packages/source-python/memory/manager.py
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -619,21 +619,34 @@ def virtual_function(
if return_type not in DataType.values:
return_type = self.create_converter(return_type)

def fget(ptr):
"""Return the virtual function."""
# Create the virtual function
func = ptr.make_virtual_function(
index,
convention,
args,
return_type
)
class fget(object):
def __set_name__(fget_self, owner, name):
fget_self.name = name

def __get__(fget_self, obj, cls=None):
"""Return the virtual function."""
if obj is None:
return fget_self

# Create the virtual function
func = obj.make_virtual_function(
index,
convention,
args,
return_type
)

# Wrap it using MemberFunction, so we don't have to pass the this
# pointer anymore
func = MemberFunction(self, return_type, func, obj)
func.__doc__ = doc

# Wrap it using MemberFunction, so we don't have to pass the this
# pointer anymore
return MemberFunction(self, return_type, func, ptr)
# Set the MemberFunction as an attribute to the instance.
setattr(obj, fget_self.name, func)

return property(fget, None, None, doc)
return func

return fget()

def function(
self, identifier, args=(), return_type=DataType.VOID,
Expand All @@ -646,34 +659,48 @@ def function(
if return_type not in DataType.values:
return_type = self.create_converter(return_type)

# Store the function cache
func = None

class fget(object):
def __set_name__(fget_self, owner, name):
fget_self.name = name

def __get__(fget_self, obj, cls=None):
nonlocal func
if cls is None:
if obj is None:
return fget_self
else:
cls = obj.__class__

if cls._binary is None:
raise ValueError('_binary was not specified.')
if func is None:
if cls._binary is None:
raise ValueError('_binary was not specified.')

# Create a binary object
binary = find_binary(cls._binary, cls._srv_check)
# Create a binary object
binary = find_binary(cls._binary, cls._srv_check)

# Create the function object
func = binary[identifier].make_function(
convention,
args,
return_type
)
# Create the function object and cache it
func = binary[identifier].make_function(
convention,
args,
return_type
)
func.__doc__ = doc

# Called with a this pointer?
if obj is not None:
# Wrap the function using MemberFunction, so we don't have
# to pass the this pointer anymore
func = MemberFunction(self, return_type, func, obj)
# Wrap the function using MemberFunction,
# so we don't have to pass the this pointer anymore
m_func = MemberFunction(self, return_type, func, obj)
m_func.__doc__ = doc

# Set the MemberFunction as an attribute to the instance.
setattr(obj, fget_self.name, m_func)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is going to cause obj to leak forever.

This comment was marked as outdated.

Sign in to view

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, that doesn't really make much sense. Forget about it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was discarded. Here are the new test results.
Output :

TypeManager.function Test
4.355441149324179 Test._spawn
4.830625455826521 test._spawn
4.801660872995853 Test()._spawn
↓
0.023133378475904465 Test._spawn
0.29126785323023796 test._spawn
0.293557733297348 Test()._spaw

TypeManager.virtual_function Test
0.5497394129633904 test.blind
0.5683586820960045 make_object(Test, pointer).blind
↓
0.46108797565102577 test.blind
0.4727563001215458 make_object(Test, pointer).blind

Copy link
Contributor

@jordanbriere jordanbriere Jun 17, 2023 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So there is no way to set MemberFunction to the instance. Can't we just use types.MethodType, for example, and set the method to the instance with a name like _function_name, with the exception for normal function call?

As you figured already, you would get the same leak with MethodType. In fact, any objects inheriting from Boost.Python.instance are never traversed for circular references. Basically, if any objects contained into its __dict__ strongly reference the object it belongs to it will be kept hostage by the circular references. For example, the following would leak:

ptr = alloc(1024)
ptr.self = ptr

Because the refcount of ptr will never reach zero, and will never be traversed by the cyclic collection. EntityMemFuncWrapper workaround this by storing a weak proxy instead of a strong reference:

Source.Python/addons/source-python/packages/source-python/entities/helpers.py

Line 104 in 12a24f4

self.wrapped_self = proxy(wrapped_self)

An easy workaround here, could be to simply set MemberFunction._this to obj's pointer instead of obj itself since that is all MemberFunction really need to forward the calls. Though, we should probably address the issue at the root, but I'm not sure of the overall implications.

EDIT: This seems to address the issue: ab33bc8

from gc import collect, is_tracked
from weakref import ref
from memory import alloc

ptr = alloc(1024)
# ptr should not be tracked, because it has no __dict__
assert not is_tracked(ptr)
ptr.self = ptr
# Now that ptr has a __dict__, it should be tracked
assert is_tracked(ptr)
r = ref(ptr)
# Our ref should be alive because ptr is still referenced
assert r()
del ptr
collect()
# Now that ptr has been deleted and collected, our ref should be dead
assert not r()

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Are there any other issues with this pull request?

I haven't tested, but based on the code, I can tell you that the _ptr() call on L628 is redundant since ptr is already a Pointer. That said, I think virtual_function could probably make great use of CachedProperty.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I haven't tested, but based on the code, I can tell you that the _ptr() call on L628 is redundant since ptr is already a Pointer.

Done.

That said, I think virtual_function could probably make great use of CachedProperty.

Using CachedProperty leaks ptr/_this. Is there a way to use CachedProperty without causing leaks?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Using CachedProperty leaks ptr/_this. Is there a way to use CachedProperty without causing leaks?

Technically this is what unbound is meant for. But now that I think about it, it seems flawed because this introduce a triangular reference that even ab33bc8 would theoretically not address.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does not work regardless of the value of unbound.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, it does not work regardless of the value of unbound.

Yeah, and it makes sense. Meaning that cached_result suffers from this regardless of 6d594fb. I don't remember specifically what testing I performed at the time that led me to believe otherwise but, if I had to guess, I probably used Player.is_bot with non-cached objects. This is misleading because it inherits from Entity.__hash__ meaning that the same value is re-used for every objects that are allocated for the same player.


return m_func

func.__doc__ = doc
return func

return fget()
Expand Down