[libvirt] [PATCH python] Optimize callback lookup in event handlers

From: "Daniel P. Berrange" <berrange@redhat.com> The event handler code currently invokes PyImport_ImportModule which is very heavyweight. This is not in fact required, since we know the libvirt module has already been imported. We can thus use PyImport_ImportModuleNoBlock and do away with the global variables caching the imported module reference. Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-override.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-) diff --git a/libvirt-override.c b/libvirt-override.c index 03aab89..aaee6b8 100644 --- a/libvirt-override.c +++ b/libvirt-override.c @@ -4929,39 +4929,26 @@ cleanup: * Helper functions to avoid importing modules * for every callback *******************************************/ -static PyObject *libvirt_module = NULL; -static PyObject *libvirt_dict = NULL; static PyObject * -getLibvirtModuleObject(void) { - if (libvirt_module) - return libvirt_module; - - // PyImport_ImportModule returns a new reference - /* Bogus (char *) cast for RHEL-5 python API brokenness */ - libvirt_module = PyImport_ImportModule((char *)"libvirt"); - if (!libvirt_module) { - DEBUG("%s Error importing libvirt module\n", __FUNCTION__); +getLibvirtDictObject(void) { + PyObject *libvirt_mod; + PyObject *libvirt_dict; + + libvirt_mod = PyImport_ImportModuleNoBlock("libvirt"); + if (!libvirt_mod) { + DEBUG("%s Error finding libvirt in imports\n", + __FUNCTION__); PyErr_Print(); return NULL; } - - return libvirt_module; -} - -static PyObject * -getLibvirtDictObject(void) { - if (libvirt_dict) - return libvirt_dict; - - // PyModule_GetDict returns a borrowed reference - libvirt_dict = PyModule_GetDict(getLibvirtModuleObject()); + libvirt_dict = PyModule_GetDict(libvirt_mod); if (!libvirt_dict) { - DEBUG("%s Error importing libvirt dictionary\n", __FUNCTION__); + DEBUG("%s Error finding libvirt dict\n", + __FUNCTION__); PyErr_Print(); return NULL; } - Py_INCREF(libvirt_dict); return libvirt_dict; } -- 1.8.3.1

On 12/03/2013 09:01 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The event handler code currently invokes PyImport_ImportModule which is very heavyweight. This is not in fact required, since we know the libvirt module has already been imported. We can thus use PyImport_ImportModuleNoBlock and do away with the global variables caching the imported module reference.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-override.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-)
- /* Bogus (char *) cast for RHEL-5 python API brokenness */ - libvirt_module = PyImport_ImportModule((char *)"libvirt"); - if (!libvirt_module) { - DEBUG("%s Error importing libvirt module\n", __FUNCTION__); +getLibvirtDictObject(void) { + PyObject *libvirt_mod; + PyObject *libvirt_dict; + + libvirt_mod = PyImport_ImportModuleNoBlock("libvirt");
Do we still need the bogus cast? (I guess I should try building libvirt-python on my RHEL-5 VM...)
if (!libvirt_dict) { - DEBUG("%s Error importing libvirt dictionary\n", __FUNCTION__); + DEBUG("%s Error finding libvirt dict\n", + __FUNCTION__);
Indentation looks off. Oh - TAB damage. Hmm - we really ought to figure out how to reinstate some syntax checking. It's probably not worth waiting for the RHEL 5 results (or, more likely, it will be more than just this area that needs patching for building libvirt-python on RHEL 5), so I'm okay with giving ACK and letting you push as is. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org

On Tue, Dec 03, 2013 at 09:36:28AM -0700, Eric Blake wrote:
On 12/03/2013 09:01 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The event handler code currently invokes PyImport_ImportModule which is very heavyweight. This is not in fact required, since we know the libvirt module has already been imported. We can thus use PyImport_ImportModuleNoBlock and do away with the global variables caching the imported module reference.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-override.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-)
- /* Bogus (char *) cast for RHEL-5 python API brokenness */ - libvirt_module = PyImport_ImportModule((char *)"libvirt"); - if (!libvirt_module) { - DEBUG("%s Error importing libvirt module\n", __FUNCTION__); +getLibvirtDictObject(void) { + PyObject *libvirt_mod; + PyObject *libvirt_dict; + + libvirt_mod = PyImport_ImportModuleNoBlock("libvirt");
Do we still need the bogus cast? (I guess I should try building libvirt-python on my RHEL-5 VM...)
I just tested the build with RHEL-5.10 and it was fine - I verified the header declares the parameter const, so presumably the flaw was fixed at some point. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|

On Wed, Dec 04, 2013 at 05:06:19PM +0000, Daniel P. Berrange wrote:
On Tue, Dec 03, 2013 at 09:36:28AM -0700, Eric Blake wrote:
On 12/03/2013 09:01 AM, Daniel P. Berrange wrote:
From: "Daniel P. Berrange" <berrange@redhat.com>
The event handler code currently invokes PyImport_ImportModule which is very heavyweight. This is not in fact required, since we know the libvirt module has already been imported. We can thus use PyImport_ImportModuleNoBlock and do away with the global variables caching the imported module reference.
Signed-off-by: Daniel P. Berrange <berrange@redhat.com> --- libvirt-override.c | 35 +++++++++++------------------------ 1 file changed, 11 insertions(+), 24 deletions(-)
- /* Bogus (char *) cast for RHEL-5 python API brokenness */ - libvirt_module = PyImport_ImportModule((char *)"libvirt"); - if (!libvirt_module) { - DEBUG("%s Error importing libvirt module\n", __FUNCTION__); +getLibvirtDictObject(void) { + PyObject *libvirt_mod; + PyObject *libvirt_dict; + + libvirt_mod = PyImport_ImportModuleNoBlock("libvirt");
Do we still need the bogus cast? (I guess I should try building libvirt-python on my RHEL-5 VM...)
I just tested the build with RHEL-5.10 and it was fine - I verified the header declares the parameter const, so presumably the flaw was fixed at some point.
Damn, I screwed up and logged into the wrong VM when testing this. It does not in fact work in RHEL-5, python 2.4, so I've reverted this commit entirely. The PyImport_ImportModuleNoBlock function does not exist. Daniel -- |: http://berrange.com -o- http://www.flickr.com/photos/dberrange/ :| |: http://libvirt.org -o- http://virt-manager.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: http://entangle-photo.org -o- http://live.gnome.org/gtk-vnc :|
participants (2)
-
Daniel P. Berrange
-
Eric Blake