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 <from...@redhat.com> Gerrit-Reviewer: Antoni Segura Puimedon <asegu...@redhat.com> Gerrit-Reviewer: Francesco Romani <from...@redhat.com> Gerrit-Reviewer: Nir Soffer <nsof...@redhat.com> Gerrit-Reviewer: Yaniv Bronhaim <ybron...@redhat.com> Gerrit-Reviewer: Yeela Kaplan <ykap...@redhat.com> Gerrit-Reviewer: automat...@ovirt.org Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list vdsm-patches@lists.fedorahosted.org https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
