Change in vdsm[master]: Avoid to recompile namedtuple ATTR classes in lvm.

Eduardo has uploaded a new change for review.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Avoid to recompile namedtuple ATTR classes in lvm.

Change-Id: I66110a7f25fb5cfd80ddffe9a22e1cbac11de447
Signed-off-by: Eduardo 
---
M vdsm/storage/lvm.py
1 file changed, 9 insertions(+), 17 deletions(-)

git pull ssh://gerrit.ovirt.org:29418/vdsm refs/changes/78/25678/1

diff --git a/vdsm/storage/lvm.py b/vdsm/storage/lvm.py
index 0fbed29..d963736 100644
--- a/vdsm/storage/lvm.py
+++ b/vdsm/storage/lvm.py
@@ -61,7 +61,9 @@

PV = namedtuple("PV", PV_FIELDS + ",guid")
 VG = namedtuple("VG", VG_FIELDS + ",writeable,partial")
+VG_ATTR = namedtuple("VG_ATTR", VG_ATTR_BITS)
 LV = namedtuple("LV", LV_FIELDS + ",writeable,opened,active")
+LV_ATTR = namedtuple("LV_ATTR", LV_ATTR_BITS)
 Stub = namedtuple("Stub", "name, stale")

@@ -185,19 +187,6 @@
     return tuple(sTags.split(",")) if sTags else tuple()

-def _attr2NamedTuple(sAttr, attrMask, label):
-    """
-    Converts a attr string into a named tuple.
-
-    Fields are named as in attrMask.
-    """
-    Attrs = namedtuple(label, attrMask)
-    # tuple("wz--n-") = ('w', 'z', '-', '-', 'n', '-')
-    values = tuple(sAttr[:len(attrMask)])
-    attrs = Attrs(*values)
-    return attrs
-
-
 def makePV(*args):
     guid = os.path.basename(args[1])
     args += (guid,)
@@ -210,8 +199,10 @@
     tags = _tags2Tuple(args[VG._fields.index("tags")])
     args[VG._fields.index("tags")] = tags
     # Convert attr string into named tuple fields.
-    attrs = _attr2NamedTuple(args[VG._fields.index("attr")], VG_ATTR_BITS,
-                             "VG_ATTR")
+    # tuple("wz--n-") = ('w', 'z', '-', '-', 'n', '-')
+    sAttr = args[VG._fields.index("attr")]
+    attr_values = tuple(sAttr[:len(VG_ATTR._fields)])
+    attrs = VG_ATTR(*attr_values)
     args[VG._fields.index("attr")] = attrs
     # Convert pv_names list to tuple.
     args[VG._fields.index("pv_name")] = \
@@ -228,8 +219,9 @@
     tags = _tags2Tuple(args[LV._fields.index("tags")])
     args[LV._fields.index("tags")] = tags
     # Convert attr string into named tuple fields.
-    attrs = _attr2NamedTuple(args[LV._fields.index("attr")], LV_ATTR_BITS,
-                             "LV_ATTR")
+    sAttr = args[LV._fields.index("attr")]
+    attr_values = tuple(sAttr[:len(LV_ATTR._fields)])
+    attrs = LV_ATTR(*attr_values)
     args[LV._fields.index("attr")] = attrs
     # Add properties. Should be ordered as VG_PROPERTIES.
     args.append(attrs.permission == "w")  # writable

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 1:

Build Failed

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6654/ : FAILURE

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7555/ : SUCCESS

Antoni Segura Puimedon has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 1: Code-Review+1

Itamar Heim has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 1:

can you please elaborate in the commit message about the purpose of the change - i.e., how/what does it improve?
thanks

[Nir Soffer]

Nir Soffer has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/25678/1/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 220:     args[LV._fields.index("tags")] = tags
Line 221:     # Convert attr string into named tuple fields.
Line 222:     sAttr = args[LV._fields.index("attr")]
Line 223:     attr_values = tuple(sAttr[:len(LV_ATTR._fields)])
Line 224:     attrs = LV_ATTR(*attr_values)
This change introduce duplication that the _attr2NamedTupple function prevented.

We can instead fix the function like this:

def _attr2NamedTupple(attr, cls):
    values = attr[:len(cls._fields)]
    return cls(*values)

What do you think?
Line 225:     args[LV._fields.index("attr")] = attrs
Line 226:     # Add properties. Should be ordered as VG_PROPERTIES.
Line 227:     args.append(attrs.permission == "w")  # writable
Line 228:     args.append(attrs.devopen == "o")     # opened

Eduardo has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 1:

(1 comment)

http://gerrit.ovirt.org/#/c/25678/1/vdsm/storage/lvm.py
File vdsm/storage/lvm.py:

Line 220:     args[LV._fields.index("tags")] = tags
Line 221:     # Convert attr string into named tuple fields.
Line 222:     sAttr = args[LV._fields.index("attr")]
Line 223:     attr_values = tuple(sAttr[:len(LV_ATTR._fields)])
Line 224:     attrs = LV_ATTR(*attr_values)
> This change introduce duplication that the _attr2NamedTupple function preve

Your suggested change only makes the code hard to follow.
The new code can be only one line then clearly is no use for a function here.

May be will be a case for unifying makeVG() and makeLV() but IMHO will make the code arcane without real gains.

_attr2NamedTuple() purpose was to deal with this attributes dynamically. After years of use we have no deal with strings like this except this two specific points.
Line 225:     args[LV._fields.index("attr")] = attrs
Line 226:     # Add properties. Should be ordered as VG_PROPERTIES.
Line 227:     args.append(attrs.permission == "w")  # writable
Line 228:     args.append(attrs.devopen == "o")     # opened

[Jenkins CI RO]

oVirt Jenkins CI Server has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 2:

Build Successful

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

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit_el/6663/ : SUCCESS

http://jenkins.ovirt.org/job/vdsm_unit_tests_gerrit/7565/ : SUCCESS

Yeela Kaplan has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 2: Code-Review+1

Francesco Romani has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 2: Code-Review+1

looks OK to me. Benchmark results will follow up ASAP.

I also think it could be merged anyway.
My only (very minor) concern is about the direct access to the _fields -in general I don't like to access private fields- but I don't see yet how to make it better.

Saggi Mizrahi has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 2: Code-Review+1

[Dan Kenigsberg]

Dan Kenigsberg has posted comments on this change.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Patch Set 2: Code-Review+2

According to
http://paste.fedoraproject.org/86373/ and http://paste.fedoraproject.org/86378/
this patch slices some 15% of the startup time of multiple concurrent block-storage-based VMs.

[Dan Kenigsberg]

Dan Kenigsberg has submitted this change and it was merged.

Change subject: Avoid to recompile namedtuple ATTR classes in lvm.
......................................................................

Avoid to recompile namedtuple ATTR classes in lvm.

Re-defining the ??_ATTR classes may be hurting the performance
during the VM creation (preliminary). This patch avoids attribute
class creation each time the lvm object is recreated.

Should be noted that re-creating lvm cached objects during the VM
creation is not required, therefore paths modified by the actual
patch should not be called in such scenario.
This abnormal situation will be corrected in a following patch.

Dealing with variable attribute strings is not required for the
actual development of oVirt and the lvm module, then
_attr2NamedTuple() can be removed.

Change-Id: I66110a7f25fb5cfd80ddffe9a22e1cbac11de447
Signed-off-by: Eduardo 
Reviewed-on: http://gerrit.ovirt.org/25678
Tested-by: Meital bourvine 
Reviewed-by: Antoni Segura Puimedon 
Reviewed-by: Yeela Kaplan 
Reviewed-by: Francesco Romani 
Reviewed-by: Saggi Mizrahi 
Reviewed-by: Dan Kenigsberg 
---
M vdsm/storage/lvm.py
1 file changed, 9 insertions(+), 17 deletions(-)

Approvals:
  Yeela Kaplan: Looks good to me, but someone else must approve
  Saggi Mizrahi: Looks good to me, but someone else must approve
  Antoni Segura Puimedon: Looks good to me, but someone else must approve
  Dan Kenigsberg: Looks good to me, approved
  Meital bourvine: Verified
  Francesco Romani: Looks good to me, but someone else must approve