Francesco Romani has posted comments on this change. Change subject: virt: faster getAllVmStats ......................................................................
Patch Set 1: (5 comments) http://gerrit.ovirt.org/#/c/35140/1//COMMIT_MSG Commit Message: Line 7: virt: faster getAllVmStats Line 8: Line 9: avoid (apparently?) wasteful work and make Line 10: the verb implementation faster with no Line 11: changes in behaviour. > It is not clear from the patch was is the wasteful work, and what is the ef We don't have a test, and I'll work to add it. I used the question mark, and do not profile it, because I was concerned about having a good reason to do all this layering traversal, a reason that I could have missed. Line 12: Line 13: Change-Id: If1ed3f49c236086c6184973762d9909d7f79bf7e http://gerrit.ovirt.org/#/c/35140/1/vdsm/API.py File vdsm/API.py: Line 1287 Line 1288 Line 1289 Line 1290 Line 1291 > Why did you replace this with self._cif.vmContainer.values()? Here and in the lines below we just want to iterate in all the VMs inside vmContainers, and this is a funny way to do that. Let's consider getVMList(), it is short enough to be reported here. Adding some annotations: def getVMList(self, fullStatus=False, vmList=()): def reportedStatus(v, full): d = v.status() # here "v" is a vdsm.virt.Vm object if full: return d else: return {'vmId': d['vmId'], 'status': d['status']} # To improve complexity, convert 'vms' to set(vms) vmSet = set(vmList) return {'status': doneCode, 'vmList': [reportedStatus(v, fullStatus) for v in self._cif.vmContainer.values() if not vmSet or v.id in vmSet]} In this case we just want all the vmIds, as we can see on the line 1294 below. Please note the waste of almost all the return value of vdsm.virt.Vm.status(). In turn, we want the vmIds just to be able to create api.VM objects (see below). Line 1290 Line 1291 Line 1292 Line 1293 Line 1294 > Why did you remove the runHooks=False argument to getStats()? Needs rebase. The original code called api.VM.getStats(). Now I'm calling vdsm.virt.Vm.getStats(), which doesn't have the hooks. the api.VM code wraps a vdsm.virt.Vm call, but here we don't need that wrapping or extra fields (statusCode), because we are going to throw it away anyway. That wrapping is useful when we ask for stats for a single Vm, not for all of them. So, instead doing extra work and discarding extra data, I'm just bypassing a layer. Line 1292 Line 1293 Line 1294 Line 1295 Line 1296 > Why did you remove the check fro falsy response? Because I find the check bogus. Looking at the implementation of api.VM.getStats(), it either returns errCode['noVM'] or a well formed response, neither of those is falsy. I just return the bare response for the reasons outlined in the comment above. http://gerrit.ovirt.org/#/c/35140/1/vdsm/virt/vm.py File vdsm/virt/vm.py: Line 2259: Please note that some values are provided by client (engine) Line 2260: but can change as result as interaction with libvirt (display*) Line 2261: """ Line 2262: stats = { Line 2263: 'vmId': self.conf['vmId'], > Is this related? It is. See comments for API.py Line 2264: 'pid': self.conf['pid'], Line 2265: 'vmType': self.conf['vmType'], Line 2266: 'kvmEnable': self._kvmEnable, Line 2267: 'acpiEnable': self.conf.get('acpiEnable', 'true')} -- To view, visit http://gerrit.ovirt.org/35140 To unsubscribe, visit http://gerrit.ovirt.org/settings Gerrit-MessageType: comment Gerrit-Change-Id: If1ed3f49c236086c6184973762d9909d7f79bf7e Gerrit-PatchSet: 1 Gerrit-Project: vdsm Gerrit-Branch: master Gerrit-Owner: Francesco Romani <[email protected]> Gerrit-Reviewer: Dan Kenigsberg <[email protected]> Gerrit-Reviewer: Francesco Romani <[email protected]> Gerrit-Reviewer: Nir Soffer <[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
