Saggi Mizrahi has posted comments on this change. Change subject: oop: Use a single instance of IOProcess per SD ......................................................................
Patch Set 4: Code-Review-1 (7 comments) http://gerrit.ovirt.org/#/c/31501/4/vdsm/storage/outOfProcess.py File vdsm/storage/outOfProcess.py: Line 46: Line 47: _oopImpl = RFH Line 48: Line 49: DEFAULT_TIMEOUT = config.getint("irs", "process_pool_timeout") Line 50: DEF_IOPROC_TIMEOUT = config.getint("irs", "ioprocess_timeout") process_pool_timeout and ioprocess_timeout are confusing as they mean completely different things. I'd call it "max_ioprocess_idle_time". Line 51: HELPERS_PER_DOMAIN = config.getint("irs", "process_pool_max_slots_per_domain") Line 52: MAX_QUEUED = config.getint("irs", "process_pool_max_queued_slots_per_domain") Line 53: Line 54: _procPoolLock = threading.Lock() Line 67: _oopImpl = RFH Line 68: Line 69: Line 70: def getProcessPool(clientName): Line 71: try: There are two different logic paths intertwined and that makes following what going on pretty hard. create _getRfhPool(clientName) and _getIOProcessPool(clientName) have getProcessPool(clientName) be def getProcessPool(clientName): if __oopImpl == IOPROC: return _getIOProcessPool(clientName) else: return _getRfhPool(clientName) That way it's simple to follow what is related to what. Line 72: if _oopImpl == IOPROC: Line 73: with _procPoolLock: Line 74: for item in _procPool.items(): Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \ Line 69: Line 70: def getProcessPool(clientName): Line 71: try: Line 72: if _oopImpl == IOPROC: Line 73: with _procPoolLock: 74-78 could be it's own function [cleanIdleIOProcesses()] to simplify things up a bit. Line 74: for item in _procPool.items(): Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \ Line 76: item[0] != clientName: Line 77: del _procPool[item[0]] Line 71: try: Line 72: if _oopImpl == IOPROC: Line 73: with _procPoolLock: Line 74: for item in _procPool.items(): Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \ You could only get time.time() once before the loop to save on system calls. In any case, it's better to use the monotonic system timer and not the system clock as the former cannot be set. you can get it with: elapsed_time = lambda : os.times()[4] Also, it's better to put the EOL time and not the last-use time. That way you leave the EOL calculation to the function that creates the instance in the list and not the one that clears it. Practically: 84: _procPool[clientName] = (elapsed_time() + DEF_IOPROCE_TIMEOUT, proc) 74: now = elapsed_time() for clientName, (eol, proc) in _procPool.items(): if (el < now): del _procPool[clientName] Line 76: item[0] != clientName: Line 77: del _procPool[item[0]] Line 78: Line 79: procref = _refProcPool[clientName] Line 75: if (time.time() - item[1][0]) > DEF_IOPROC_TIMEOUT and \ Line 76: item[0] != clientName: Line 77: del _procPool[item[0]] Line 78: Line 79: procref = _refProcPool[clientName] you could just do: proc = _refProcPool[clientName]() Line 80: proc = procref() Line 81: if proc is None: Line 82: raise KeyError Line 83: else: Line 77: del _procPool[item[0]] Line 78: Line 79: procref = _refProcPool[clientName] Line 80: proc = procref() Line 81: if proc is None: We don't want another leak. del _refProcPool[clientName] Line 82: raise KeyError Line 83: else: Line 84: _procPool[clientName] = (time.time(), proc) Line 85: return proc Line 90: if _oopImpl == IOPROC: Line 91: proc = _OopWrapper(IOProcess(max_threads=HELPERS_PER_DOMAIN, Line 92: timeout=DEFAULT_TIMEOUT, Line 93: max_queued_requests=MAX_QUEUED)) Line 94: _procPool[clientName] = (time.time(), proc) See previous comments Line 95: _refProcPool[clientName] = weakref.ref(proc) Line 96: procref = _refProcPool[clientName] Line 97: Line 98: return procref() -- To view, visit http://gerrit.ovirt.org/31501 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: I383fe617ee4ce22de368ba54f980887d70ff37c5 Gerrit-PatchSet: 4 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Yeela Kaplan <[email protected]> Gerrit-Reviewer: Antoni Segura Puimedon <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Dima Kuznetsov <[email protected]> Gerrit-Reviewer: Federico Simoncelli <[email protected]> Gerrit-Reviewer: Saggi Mizrahi <[email protected]> Gerrit-Reviewer: Yaniv Bronhaim <[email protected]> Gerrit-Reviewer: Yeela Kaplan <[email protected]> Gerrit-Reviewer: [email protected] Gerrit-Reviewer: oVirt Jenkins CI Server Gerrit-HasComments: Yes _______________________________________________ vdsm-patches mailing list [email protected] https://lists.fedorahosted.org/mailman/listinfo/vdsm-patches
