Change in vdsm[master]: profiling: Add support for memory profiling
fromani at redhat.com
fromani at redhat.com
Tue Dec 9 09:08:14 UTC 2014
Francesco Romani has posted comments on this change.
Change subject: profiling: Add support for memory profiling
......................................................................
Patch Set 4:
(10 comments)
http://gerrit.ovirt.org/#/c/32019/4/lib/vdsm/profile_mem.py
File lib/vdsm/profile_mem.py:
Line 47:
Line 48:
Line 49: def start():
Line 50: """ Starts application memory profiling """
Line 51: if _SUPPORTED and is_enabled():
> Only now we know that we can run the memory profiler - so only at this poin
Done
Line 52: _start_profiling()
Line 53:
Line 54:
Line 55: def stop():
Line 48:
Line 49: def start():
Line 50: """ Starts application memory profiling """
Line 51: if _SUPPORTED and is_enabled():
Line 52: _start_profiling()
> This also must raise if already started, the caller should be punished for
Done
Line 53:
Line 54:
Line 55: def stop():
Line 56: """ Stops application memory profiling """
Line 54:
Line 55: def stop():
Line 56: """ Stops application memory profiling """
Line 57: if is_running():
Line 58: _stop_profiling()
> I think we should inline _stop_profiling here:
Good point. Rearranged code to use lock and to fix this issue.
Line 59:
Line 60:
Line 61: def is_enabled():
Line 62: return config.getboolean('vars', 'profile_memory_enable')
Line 62: return config.getboolean('vars', 'profile_memory_enable')
Line 63:
Line 64:
Line 65: def is_running():
Line 66: return _thread is not None and _thread.is_alive()
> What if thread was created but is not running yet? Should be simply:
Done
Line 67:
Line 68:
Line 69: def _memory_viewer(port=None):
Line 70: cherrypy.tree.mount(dowser.Root())
Line 65: def is_running():
Line 66: return _thread is not None and _thread.is_alive()
Line 67:
Line 68:
Line 69: def _memory_viewer(port=None):
> Why do we need configurable port? we have configuration for this.
Good point. Reorganized code to fix this and remove unneeded 'port' argument: configuration item is good enough.
Line 70: cherrypy.tree.mount(dowser.Root())
Line 71:
Line 72: port = (config.getint('vars', 'profile_memory_port')
Line 73: if port is None else port)
Line 79: cherrypy.engine.start()
Line 80:
Line 81:
Line 82: def _start_profiling():
Line 83: global _thread
> Raise if already running
Done - but with new code flow.
Line 84: _thread = threading.Thread(name='memprofile', target=_memory_viewer)
Line 85: _thread.daemon = True
Line 86: _thread.start()
Line 87:
Line 82: def _start_profiling():
Line 83: global _thread
Line 84: _thread = threading.Thread(name='memprofile', target=_memory_viewer)
Line 85: _thread.daemon = True
Line 86: _thread.start()
> Can be inlined into start(), or renamed to _start_profiling_thread
Done in a different way.
Line 87:
Line 88:
Line 89: def _stop_profiling():
Line 90: global _thread
Line 89: def _stop_profiling():
Line 90: global _thread
Line 91: cherrypy.engine.exit()
Line 92: cherrypy.engine.block()
Line 93: _thread.join()
> _thread may be None - this force the caller to invoke is_running.
Done
Line 90: global _thread
Line 91: cherrypy.engine.exit()
Line 92: cherrypy.engine.block()
Line 93: _thread.join()
Line 94: del _thread
> Now is_running will fail with NameError. Should be:
Done
http://gerrit.ovirt.org/#/c/32019/4/vdsm/vdsm
File vdsm/vdsm:
Line 69: signal.signal(signal.SIGUSR1, sigusr1Handler)
Line 70: zombiereaper.registerSignalHandler()
Line 71:
Line 72: profile.start()
Line 73: profile_mem.start()
> Lets replace this with:
Done
Line 74:
Line 75: libvirtconnection.start_event_loop()
Line 76:
Line 77: if config.getboolean('irs', 'irs_enable'):
--
To view, visit http://gerrit.ovirt.org/32019
To unsubscribe, visit http://gerrit.ovirt.org/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Ib56b65513e0118b68cd43791bf655c928d6a26e2
Gerrit-PatchSet: 4
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <asegurap at redhat.com>
Gerrit-Reviewer: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Nir Soffer <nsoffer at redhat.com>
Gerrit-Reviewer: Yaniv Bronhaim <ybronhei at redhat.com>
Gerrit-Reviewer: Yeela Kaplan <ykaplan at redhat.com>
Gerrit-Reviewer: automation at ovirt.org
Gerrit-Reviewer: oVirt Jenkins CI Server
Gerrit-HasComments: Yes
More information about the vdsm-patches
mailing list