Change in vdsm[master]: profiling: add support for memory profiling

fromani at redhat.com fromani at redhat.com
Tue Feb 17 10:51:13 UTC 2015


Francesco Romani has posted comments on this change.

Change subject: profiling: add support for memory profiling
......................................................................


Patch Set 10:

(4 comments)

http://gerrit.ovirt.org/#/c/32019/10/lib/vdsm/profiling/memory.py
File lib/vdsm/profiling/memory.py:

Line 45: 
Line 46: 
Line 47: def stop():
Line 48:     """ Stops application memory profiling """
Line 49:     if is_running():
> We don't need this check, and its racy.
will remove.
Line 50:         _stop_profiling()
Line 51: 
Line 52: 
Line 53: def is_enabled():


Line 54:     return config.getboolean('devel', 'memory_profile_enable')
Line 55: 
Line 56: 
Line 57: def _is_alive():
Line 58:     return _thread is not None
> I think that checking _thread is good enough. We don't need both is_running
will rename is_alive as is_running
Line 59: 
Line 60: 
Line 61: def is_running():
Line 62:     with _lock:


Line 79:     global _thread
Line 80:     logging.debug("Starting memory profiling")
Line 81:     with _lock:
Line 82:         import cherrypy
Line 83:         import dowser
> Importing stuff under a lock may lead to deadlocks - I would do the import 
Oops. Thanks for pointing this out. Will fix.
Line 84:         if _is_alive():
Line 85:             raise UsageError('Memory profiler is already running')
Line 86:         _thread = threading.Thread(name='memprofile',
Line 87:                                    target=_memory_viewer)


http://gerrit.ovirt.org/#/c/32019/10/lib/vdsm/profiling/profile.py
File lib/vdsm/profiling/profile.py:

Line 40:     return cpu.is_enabled() or memory.is_enabled()
Line 41: 
Line 42: 
Line 43: def is_running():
Line 44:     return cpu.is_running() or memory.is_running()
> Since we have multiple profilers that may be in different states, if we wan
Good point, will add.


-- 
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: 10
Gerrit-Project: vdsm
Gerrit-Branch: master
Gerrit-Owner: Francesco Romani <fromani at redhat.com>
Gerrit-Reviewer: Antoni Segura Puimedon <toni+ovirt at midokura.com>
Gerrit-Reviewer: Dan Kenigsberg <danken at redhat.com>
Gerrit-Reviewer: Federico Simoncelli <fsimonce 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