Minor efficiency improvements to ResourceGroup

- Don't call nested() multiple times in a (ahem) nested loop for no reason.
- No need to render a generator to a list when it's only going to be used
  once.
- Don't do repeated lookups in a list to check for membership; use a set.

Change-Id: Ieba934fc7a420147fa5b8dadd067a263e78f3a29

heat/engine/resources/openstack/heat/resource_group.py

@@ -13,6 +13,7 @@
 
 import collections
 import copy
+import itertools
 
 import six
 
@@ -216,6 +217,8 @@ class ResourceGroup(stack_resource.StackResource):
     def _name_blacklist(self):
         """Resolve the remove_policies to names for removal."""
 
+        nested = self.nested()
+
         # To avoid reusing names after removal, we store a comma-separated
         # blacklist in the resource data
         db_rsrc_names = self.data().get('name_blacklist')
@@ -226,21 +229,21 @@ class ResourceGroup(stack_resource.StackResource):
 
         # Now we iterate over the removal policies, and update the blacklist
         # with any additional names
-        rsrc_names = list(current_blacklist)
+        rsrc_names = set(current_blacklist)
         for r in self.properties[self.REMOVAL_POLICIES]:
             if self.REMOVAL_RSRC_LIST in r:
                 # Tolerate string or int list values
                 for n in r[self.REMOVAL_RSRC_LIST]:
                     str_n = six.text_type(n)
-                    if str_n in self.nested() and str_n not in rsrc_names:
-                        rsrc_names.append(str_n)
+                    if str_n in nested:
+                        rsrc_names.add(str_n)
                         continue
-                    rsrc = self.nested().resource_by_refid(str_n)
-                    if rsrc and str_n not in rsrc_names:
-                        rsrc_names.append(rsrc.name)
+                    rsrc = nested.resource_by_refid(str_n)
+                    if rsrc:
+                        rsrc_names.add(rsrc.name)
 
         # If the blacklist has changed, update the resource data
-        if rsrc_names != current_blacklist:
+        if rsrc_names != set(current_blacklist):
             self.data_set('name_blacklist', ','.join(rsrc_names))
         return rsrc_names
 
@@ -248,16 +251,14 @@ class ResourceGroup(stack_resource.StackResource):
         name_blacklist = self._name_blacklist()
         req_count = self.properties.get(self.COUNT)
 
-        def gen_names():
-            count = 0
-            index = 0
-            while count < req_count:
-                if str(index) not in name_blacklist:
-                    yield str(index)
-                    count += 1
-                index += 1
+        def is_blacklisted(name):
+            return name in name_blacklist
 
-        return list(gen_names())
+        candidates = itertools.imap(six.text_type, itertools.count())
+
+        return itertools.islice(itertools.ifilterfalse(is_blacklisted,
+                                                       candidates),
+                                req_count)
 
     def handle_create(self):
         names = self._resource_names()

heat/tests/test_resource_group.py

@@ -345,12 +345,18 @@ class ResourceGroupTest(common.HeatTestCase):
     def test_child_template(self):
         stack = utils.parse_stack(template2)
         snip = stack.t.resource_definitions(stack)['group1']
+
+        def check_res_names(names):
+            self.assertEqual(list(names), ['0', '1'])
+            return 'tmpl'
+
         resgrp = resource_group.ResourceGroup('test', snip, stack)
-        resgrp._assemble_nested = mock.Mock(return_value='tmpl')
+        resgrp._assemble_nested = mock.Mock()
+        resgrp._assemble_nested.side_effect = check_res_names
         resgrp.properties.data[resgrp.COUNT] = 2
 
         self.assertEqual('tmpl', resgrp.child_template())
-        resgrp._assemble_nested.assert_called_once_with(['0', '1'])
+        self.assertEqual(1, resgrp._assemble_nested.call_count)
 
     def test_child_params(self):
         stack = utils.parse_stack(template2)
@@ -370,10 +376,10 @@ class ResourceGroupBlackList(common.HeatTestCase):
     # 6) resource_list (refid) in nested() -> saved
     scenarios = [
         ('1', dict(data_in=None, rm_list=[],
-                   nested_rsrcs={}, expected=[],
+                   nested_rsrcs=[], expected=[],
                    saved=False)),
         ('2', dict(data_in='0,1,2', rm_list=[],
-                   nested_rsrcs={}, expected=['0', '1', '2'],
+                   nested_rsrcs=[], expected=['0', '1', '2'],
                    saved=False)),
         ('3', dict(data_in='1,3', rm_list=['6'],
                    nested_rsrcs=['0', '1', '3'],
@@ -421,13 +427,15 @@ class ResourceGroupBlackList(common.HeatTestCase):
 
         nested = mock.MagicMock()
         nested.__contains__.side_effect = stack_contains
+        nested.__iter__.side_effect = iter(self.nested_rsrcs)
         nested.resource_by_refid.side_effect = by_refid
         resg.nested = mock.Mock(return_value=nested)
 
-        self.assertEqual(self.expected, resg._name_blacklist())
+        blacklist = resg._name_blacklist()
+        self.assertEqual(set(self.expected), blacklist)
         if self.saved:
             resg.data_set.assert_called_once_with('name_blacklist',
-                                                  ','.join(self.expected))
+                                                  ','.join(blacklist))
 
 
 class ResourceGroupEmptyParams(common.HeatTestCase):
@@ -495,7 +503,7 @@ class ResourceGroupNameListTest(common.HeatTestCase):
         resg.properties = mock.MagicMock()
         resg.properties.get.return_value = self.count
         resg._name_blacklist = mock.MagicMock(return_value=self.blacklist)
-        self.assertEqual(self.expected, resg._resource_names())
+        self.assertEqual(self.expected, list(resg._resource_names()))
 
 
 class ResourceGroupAttrTest(common.HeatTestCase):