Added lazy-loading support to ListenerManager. #307
Conversation
Made OnEntityOutput lazy-loaded to fix loading conflict with SourceMod (see #306).
| return | ||
|
|
||
| # Unregister the hook on fire_output | ||
| fire_output.remove_pre_hook(_pre_fire_output) |
There was a problem hiding this comment.
This actually doesn't remove the hook, but just the hook handler.
Not really, as it would require the insertion of an extra wrapper class in the hierarchy, which would unnecessary adds an extra layer to the MRO. Unless we implement lazy-loading for listeners on the c++ side (e.g. the player commands listeners), we don't really need them to be virtual and simple static methods that keep the back reference is sufficient enough. Perhaps that is something we could consider, but for now, my current goal was to fix the conflict that seemed to be getting popular and that, with minimal efforts. 😄 |
| @@ -281,6 +282,36 @@ class OnClientSettingsChanged(ListenerManagerDecorator): | |||
| manager = on_client_settings_changed_listener_manager | |||
|
|
|||
|
|
|||
| class OnEntityOutpuListenerManager(ListenerManager): | |||
Fixed a typo (thank @CookStar). Added missing entry to __all__ declaration.
I'm fine with the extra layer, because it doesn't has any downsides. Moreover, I would prefer a well-thought solution instead of a quick and dirty one. |
The main downside would be that every manager would now become wrappers which means that any attribute retrieval from them would become slightly slower since the wrapper class would need to be looked up first before reaching the base class (for instance, every To me it comes to: do we intend to use that for any listeners managed on the c++ side or not. If we do, then yes, that layer becomes a necessity. But if we don't, it becomes unnecessary computation (which can adds up, considering some of them are noisy and notified multiple times per frame).
That's a concept; lazy-loading lazily implemented. 😜 |
|
Just tested the performance of both implementations with one million notify() calls. Unexpectedly, your Python implementation was slightly slower (but the difference is neglegable). The Python implementation has been tested with the following code: import time
from listeners import ListenerManager
ITERATIONS = 1_000_000
manager = ListenerManager()
now = time.time()
for x in range(ITERATIONS):
manager.notify()
print('No initialize/finalize: ', time.time() - now)
def init_fin(self):
pass
manager.initialize = init_fin
manager.finalize = init_fin
now = time.time()
for x in range(ITERATIONS):
manager.notify()
print('With initialize/finalize: ', time.time() - now)The C++ implementation with the following code: import time
from listeners import ListenerManager
ITERATIONS = 1_000_000
manager = ListenerManager()
now = time.time()
for x in range(ITERATIONS):
manager.notify()
print('No initialize/finalize: ', time.time() - now)
class MyManager(ListenerManager):
def initialize(self):
pass
def finalize(self):
pass
manager = MyManager()
now = time.time()
for x in range(ITERATIONS):
manager.notify()
print('With initialize/finalize: ', time.time() - now) |
|
Where is your code? The simple fact of inserting a dummy wrapper so that we can access the overrides: class CListenerManagerWrapper: public CListenerManager, public wrapper<CListenerManager>
{
};
//-----------------------------------------------------------------------------
// Exports CListenerManager.
//-----------------------------------------------------------------------------
void export_listener_managers(scope _listeners)
{
class_<CListenerManagerWrapper, boost::noncopyable>("ListenerManager")And running the following code: import time
from listeners import ListenerManager
ITERATIONS = 1_000_000
manager = ListenerManager()
now = time.time()
for x in range(ITERATIONS):
manager.notify()
print('No initialize/finalize: ', time.time() - now)I get the following time: |
|
You don't need the CListenerManagerWrapper class. I will push my code later today. |
|
Waiting for it. Really curious to see how you manage to dispatch the calls back to the same Python object without using a static dispatcher like I did or a wrapper class. 🤔 Besides, I just noticed: def init_fin(self):
pass
manager.initialize = init_fin
manager.finalize = init_finThere is no reason to do that. By doing so, you bind the methods to the instance, meaning its |
Hmm, I somehow thought you were binding the fire_output initializer/finalizer like that, but I just had a second look and it's not the case. Not sure why I thought so at first. Here is the C++ implementation: 12a7da7 |
I guess because the first changes listed are the dynamic definition of the method itself either to a
Right, so you ARE using a wrapper class, just not on a subclass. Which actually, is interesting. I always was under the impression that the wrapper itself was the hungry one, but seems like it is the subclass after all. In average, it seems slightly slower to resolve names, but the difference is more than acceptable. Ran the following code: import time
from listeners import ListenerManager
ITERATIONS = 1_000_000
manager = ListenerManager()
for i in range(100):
now = time.time()
for x in range(ITERATIONS):
manager.notify()
print(time.time() - now)And here are the results: With wrapperWithout wrapperNot much of a difference, but without seems to bounce a tiny bit lower from time to time regardless. Which anyways, makes no difference. I went ahead and replaced the static dispatchers here directly. Thanks, that was constructive! |
Removed redundant attribute retrievals.
|
I guess the wrapper isn't the hungry one, because it's just a subclass on the C++ side and doesn't really add stuff to the MRO on the Python side. I did not say that you don't need a wrapper class. I just meant that CListenerManagerWrapper is not required. And that statement was related to the additional subclass :P |
Look at the example I posted above, there is no extra Python class involved, just an extra C++ base and there is no On the other hand, the |
|
Alright, so I made some investigating because it wasn't making any sense to me and here are my results. Here is the Python code used to time everything: from time import time
from test import Bar
bar = Bar()
now = time()
for i in range(10000000):
bar.foo
print('R:', time() - now)
now = time()
for i in range(10000000):
bar.foo()
print('C:', time() - now)Fast: class Foo
{
public:
void foo() {}
};
class Bar: public Foo
{
};
DECLARE_SP_MODULE(test)
{
// Using derived class namespace to bind base method
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.7031171321868896
// C: 1.5408985614776611Fast: class Foo
{
public:
void foo() {}
};
class Bar: public Foo
{
};
DECLARE_SP_MODULE(test)
{
// Using base class namespace to bind base method
class_<Bar, boost::noncopyable>("Bar").def("foo", &Foo::foo);
}
// R: 0.749993085861206
// C: 1.5877742767333984Slow: // Declaring base as wrapper
class Foo: public wrapper<Foo>
{
public:
void foo() {}
};
class Bar: public Foo
{
};
DECLARE_SP_MODULE(test)
{
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.7160553932189941
// C: 3.84773325920105Slow: class Foo
{
public:
void foo() {}
};
// Declaring base as wrapper, on derived definition
class Bar: public Foo, public wrapper<Foo>
{
};
DECLARE_SP_MODULE(test)
{
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.7290487289428711
// C: 3.872662305831909Fast: // Declaring derived as wrapper, on base definition
class Bar;
class Foo: public wrapper<Bar>
{
public:
void foo() {}
};
class Bar: public Foo
{
};
DECLARE_SP_MODULE(test)
{
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.709097146987915
// C: 1.5338754653930664Fast: class Foo: public wrapper<Foo>
{
public:
void foo() {}
};
class Bar: public Foo
{
public:
// Upcasting ourselves, then forwarding the call
void foo() { ((Foo *)this)->foo(); }
};
DECLARE_SP_MODULE(test)
{
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.7091226577758789
// C: 1.5947341918945312Fast: class Foo: public wrapper<Foo>
{
};
class Bar: public Foo
{
public:
// Defining the method directly in the derived class of a wrapper
void foo() {}
};
DECLARE_SP_MODULE(test)
{
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.6931755542755127
// C: 1.5618219375610352Fast: class Foo: public wrapper<Foo>
{
public:
void foo() {}
};
class Bar: public Foo
{
public:
void foo() {}
};
DECLARE_SP_MODULE(test)
{
// Declaring an overload from derived using derived namespace of a wrapper class
class_<Bar, boost::noncopyable>("Bar").def("foo", &Bar::foo);
}
// R: 0.7798867225646973
// C: 1.5857844352722168Slow: class Foo: public wrapper<Foo>
{
public:
void foo() {}
};
class Bar: public Foo
{
public:
void foo() {}
};
DECLARE_SP_MODULE(test)
{
// Declaring an overload from derived using base wrapper class namespace
class_<Bar, boost::noncopyable>("Bar").def("foo", &Foo::foo);
}
// R: 0.7021214962005615
// C: 3.7878899574279785Conclusion: It is not the resolution that is affected, but the calls themselves. At least, any call from an object of a wrapper class that have to upcast the this pointer appears to be ~60% slower. 🤔 |
This PR adds lazy-loading support to ListenerManager so that hooks are only registered when a callback is first registered (to fix conflict with SourceMod, see #306).
The implementation is quite simple, when the first callback is registered,
ListenerManager.initializeis called andListenerManager.finalizeis called when the last one is unregistered allowing us to add/remove our hooks only when requested.