Change in vdsm[master]: vm: faster retrieval of graphics stats

Francesco Romani has uploaded a new change for review.

Change subject: vm: faster retrieval of graphics stats
......................................................................

vm: faster retrieval of graphics stats

Device object and device configuration
data is kept in sync, so there is no need to scan all the
device configuration data to report it to Engine.

Instead, just iterate on the Graphics Device classes.

Change-Id: I337e2baa5a349c2e750f883c4ca697aaafc55bf3
Related-To: https://bugzilla.redhat.com/1177634
Signed-off-by: Francesco Romani 
---
M vdsm/virt/vm.py
1 file changed, 3 insertions(+), 4 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/91/36791/1

diff --git a/vdsm/virt/vm.py b/vdsm/virt/vm.py
index 2883517..5b0881b 100644
--- a/vdsm/virt/vm.py
+++ b/vdsm/virt/vm.py
@@ -1973,17 +1973,16 @@
     def _getGraphicsStats(self):
         def getInfo(dev):
             return {
-                'type': dev['device'],
+                'type': dev.device,
                 'port': dev.get(
                     'port', GraphicsDevice.LIBVIRT_PORT_AUTOSELECT),
                 'tlsPort': dev.get(
                     'tlsPort', GraphicsDevice.LIBVIRT_PORT_AUTOSELECT),
-                'ipAddress': dev.get('specParams', {}).get('displayIp', '0')}
+                'ipAddress': dev.specParams.get('displayIp', '0')}

return {
             'displayInfo': [getInfo(dev)
-                            for dev in self.conf.get('devices', [])
-                            if dev['type'] == hwclass.GRAPHICS],
+                            for dev in self._devices[hwclass.GRAPHICS]],
             'displayType': self.conf['display'],
             'displayPort': self.conf['displayPort'],
             'displaySecurePort': self.conf['displaySecurePort'],

[e-mail] has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 1:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[e-mail] has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 2:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 1:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14977/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14808/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2165/ : There was an infra issue, please contact [e-mail]

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14020/ : FAILURE

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 2:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/14978/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14809/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2166/ : There was an infra issue, please contact [e-mail]

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14021/ : SUCCESS

[e-mail] has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Francesco Romani has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3:

rebased

Francesco Romani has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3:

rebased. Addressed Dan's comment.

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15033/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14864/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2174/ : There was an infra issue, please contact [e-mail]

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14076/ : FAILURE

Francesco Romani has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3: Verified+1

Verified running a VM with SPICE and VNC (of course two separate runs) and checked that correct output is still present on getAllVmStats.

Verified with profile.

As expected Preliminary profiling confirms savings are there, although tiny, borderline neigligeble. Function cost dropped by ~40% (from ~10s to ~6s). Total savings are in the order of <1%. (~10s over ~1900s)

However this patch is still beneficial because in the not so distant future we want to sanitize the device object vs configuration mess,
so we'll need to fix this code anyway.

[Martin Polednik]

Martin Polednik has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3: Code-Review+1

and as a bonus, the code looks cleaner

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: faster retrieval of graphics stats
......................................................................

Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/36791/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-01-12 10:46:37 +0100
Line 4: Commit:     Francesco Romani 
Line 5: CommitDate: 2015-01-14 14:05:16 +0100
Line 6: 
Line 7: vm: faster retrieval of graphics stats
In a comment to the reviewed you placed some hard data stating that the improvement is minute, and that the main motivation for this patch is cleanliness.

Please state it in the commit message.
Line 8: 
Line 9: Device object and device configuration
Line 10: data is kept in sync, so there is no need to scan all the
Line 11: device configuration data to report it to Engine.

Line 6: 
Line 7: vm: faster retrieval of graphics stats
Line 8: 
Line 9: Device object and device configuration
Line 10: data is kept in sync, so there is no need to scan all the
I'm not into data-datum nagging! but in this case we have two sets of data that ARE kept in sync.
Line 11: device configuration data to report it to Engine.
Line 12: 
Line 13: Instead, just iterate on the Graphics Device classes.
Line 14:

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

[e-mail] has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 4:

* Update tracker::IGNORE, no Bug-Url found
* Check Bug-Url::WARN, no bug url found, make sure header matches 'Bug-Url: ' and is a valid url.
* Check merged to previous::IGNORE, Not in stable branch (['ovirt-3.5', 'ovirt-3.4', 'ovirt-3.3'])

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

Francesco Romani has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 3:

(2 comments)

http://gerrit.ovirt.org/#/c/36791/3//COMMIT_MSG
Commit Message:

Line 3: AuthorDate: 2015-01-12 10:46:37 +0100
Line 4: Commit:     Francesco Romani 
Line 5: CommitDate: 2015-01-14 14:05:16 +0100
Line 6: 
Line 7: vm: faster retrieval of graphics stats
> In a comment to the reviewed you placed some hard data stating that the imp

You are of course right. Done
Even thought, it is actually faster in microbenchmarks :)
Line 8: 
Line 9: Device object and device configuration
Line 10: data is kept in sync, so there is no need to scan all the
Line 11: device configuration data to report it to Engine.

Line 6: 
Line 7: vm: faster retrieval of graphics stats
Line 8: 
Line 9: Device object and device configuration
Line 10: data is kept in sync, so there is no need to scan all the
> I'm not into data-datum nagging! but in this case we have two sets of data

Fixed. And, by the way, this kind of nitpicking is always welcome (in the end, hopefully, I'll learn).
Line 11: device configuration data to report it to Engine.
Line 12: 
Line 13: Instead, just iterate on the Graphics Device classes.
Line 14:

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

Francesco Romani has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 4: Verified+1

Rebased, copied score.

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

Francesco Romani has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 4:

...and fixed commit message.

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 4:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_created/15051/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_pep8_gerrit/14882/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_virt_functional_tests_gerrit/2180/ : There was an infra issue, please contact [e-mail]

http://jenkins.ovirt.org/job/vdsm_master_unit_tests_gerrit_el/14094/ : FAILURE

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 4: Code-Review+2

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

vm: cleaner retrieval of graphics stats

Device object and device configuration
data are kept in sync, so there is no need to scan all the
device configuration data to report it to Engine.

Instead, just iterate on the Graphics Device classes.
This new code is also a bit faster in the microbenchmarks, but
since this method is already very low on profile, total
performance gain is negligible.

Change-Id: I337e2baa5a349c2e750f883c4ca697aaafc55bf3
Related-To: https://bugzilla.redhat.com/1177634
Signed-off-by: Francesco Romani 
Reviewed-on: http://gerrit.ovirt.org/36791
Reviewed-by: Dan Kenigsberg 
---
M vdsm/virt/vm.py
1 file changed, 5 insertions(+), 8 deletions(-)

Approvals:
  Dan Kenigsberg: Looks good to me, approved
  Francesco Romani: Verified

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

[e-mail] has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 5:

* Update tracker::IGNORE, no Bug-Url found
* Set MODIFIED::IGNORE, no Bug-Url found.

Change in vdsm[master]: vm: cleaner retrieval of graphics stats

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: vm: cleaner retrieval of graphics stats
......................................................................

Patch Set 5:

Build Failed

http://jenkins.ovirt.org/job/vdsm_master_verify-error-codes_merged/6312/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el7-x86_64_merged/491/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-el6-x86_64_merged/490/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc20-x86_64_merged/485/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master_create-rpms-fc21-x86_64_merged/467/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el7-x86_64_mer... : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-el6-x86_64_mer... : ABORTED

http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc20-x86_64_me... : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master-libgfapi_create-rpms-fc21-x86_64_me... : SUCCESS

http://jenkins.ovirt.org/job/vdsm_master_unit-tests_merged/4477/ : SUCCESS