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

nsoffer at redhat.com nsoffer at redhat.com
Tue Sep 9 08:22:07 UTC 2014


Nir Soffer has posted comments on this change.

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


Patch Set 4: Code-Review-1

(10 comments)

http://gerrit.ovirt.org/#/c/32019/4/lib/vdsm/profile_mem.py
File lib/vdsm/profile_mem.py:

Line 45: 
Line 46: _thread = None
Line 47: 
Line 48: 
Line 49: def start():
This will be called when vdsm starts without checking anything.
Line 50:     """ Starts application memory profiling """
Line 51:     if _SUPPORTED and is_enabled():
Line 52:         _start_profiling()
Line 53: 


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 point we are allowed to import stuff such as cherrypy and PIL.

We don't want to modify vdsm runtime environment in any way if the profiler is not needed.

So we can do:
- Defer the import like cpu profiler does
- Or add another layer, checking the configuration and then importing and starting all enabled profiles.

I think the first option is the simplest one.

We can do this:

    if enabled():
        import ...

And let the import failure crash the process. This is the simplest way, making it easy to understand why the profiler does not work when you have missing dependency, and requires not code

Or:

    if enabled():
        try:
            import ...
        except ImportError as e:
            logging.warning("Error strarting memory profiler: %s", e)

This will not crash vdsm and provide good error message.

If you go with the second way please also modify the cpu profiler to work in the same way.
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 this.
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:

   def stop():
       if _thread is not None:
           stop cherrypy...
           _thread.join()
           _thread = None

And of course we need a lock for this - otherwise if two thread try to stop this at the same time, both may enter the function, and then one of them will fail trying to invoke None.join()
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:

    _thread is not None
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.

Also it is much clear to do:

    if port is None:
        port = config.getint(...)

Instead of the "modern" if .. else dance.

And of course do this first thing in the function. Why do you want to keep the port argument in a half baked state until the middle of the function?

It is good practice to define arguments just before you need them, so accessing them before they are defined fails. It is worst practice to have an argument defined at the start, and initialized only later.
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
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
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.


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:

    _thread = None


-- 
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