Yeela Kaplan has posted comments on this change. Change subject: oop: Use a single instance of IOProcess per SD ......................................................................
Patch Set 4: (10 comments) http://gerrit.ovirt.org/#/c/31501/4/lib/vdsm/config.py.in File lib/vdsm/config.py.in: Line 318: 'Process pool configuration.'), Line 319: Line 320: ('process_pool_timeout', '60', None), Line 321: Line 322: ('ioprocess_timeout', '60', 'TTL of IOProcess that is not renewed'), > TTL of an unused IOProcess instance Done Line 323: Line 324: ('process_pool_max_slots_per_domain', '10', None), Line 325: Line 326: ('process_pool_max_queued_slots_per_domain', '10', None), 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 compl Done 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 wh Done 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 thing Done 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 Done 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: Done 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. Done Line 82: raise KeyError Line 83: else: Line 84: _procPool[clientName] = (time.time(), proc) Line 85: return proc Line 83: else: Line 84: _procPool[clientName] = (time.time(), proc) Line 85: return proc Line 86: else: Line 87: return _rfhPool[clientName] > don't you need that also under the _procPoolLock? Done Line 88: except KeyError: Line 89: with _procPoolLock: Line 90: if _oopImpl == IOPROC: Line 91: proc = _OopWrapper(IOProcess(max_threads=HELPERS_PER_DOMAIN, 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 Done Line 95: _refProcPool[clientName] = weakref.ref(proc) Line 96: procref = _refProcPool[clientName] Line 97: Line 98: return procref() Line 99: else: Line 100: _rfhPool[clientName] = _OopWrapper( Line 101: RemoteFileHandlerPool(HELPERS_PER_DOMAIN)) Line 102: Line 103: return _rfhPool[clientName] > i'd put all of it under "with _procPoolLock" Done Line 104: Line 105: Line 106: def getGlobalProcPool(): Line 107: return getProcessPool(GLOBAL) -- 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
