From 98875345c20851dc9cea590dc86bf785db018e80 Mon Sep 17 00:00:00 2001 From: houming-wang Date: Thu, 3 Dec 2015 08:16:07 -0500 Subject: [PATCH] Performance: leverage dict comprehension in PEP-0274 PEP-0274 introduced dict comprehensions to replace dict constructor with a sequence of length-2 sequences, these are benefits copied from [1]: The dictionary constructor approach has two distinct disadvantages from the proposed syntax though. First, it isn't as legible as a dict comprehension. Second, it forces the programmer to create an in-core list object first, which could be expensive. Magnum does not support python 2.6, we can leverage this. There is deep dive about PEP-0274[2] and basic tests about performance[3]. Note: This commit doesn't handle dict constructor with kwagrs. This commit also adds a hacking rule. [1]http://legacy.python.org/dev/peps/pep-0274/ [2]http://doughellmann.com/2012/11/12/the-performance-impact-of-using -dict-instead-of-in-cpython-2-7-2.html [3]http://paste.openstack.org/show/480757/ Change-Id: I61992fa428d6760449afe3754b02506336e8b421 --- HACKING.rst | 2 ++ magnum/api/controllers/base.py | 8 ++++---- magnum/cmd/template_manage.py | 2 +- magnum/hacking/checks.py | 10 ++++++++++ magnum/objects/base.py | 6 +++--- magnum/tests/unit/api/utils.py | 2 +- magnum/tests/unit/test_hacking.py | 28 ++++++++++++++++++++++++++++ 7 files changed, 49 insertions(+), 9 deletions(-) diff --git a/HACKING.rst b/HACKING.rst index 99d89d30cf..02b661526b 100644 --- a/HACKING.rst +++ b/HACKING.rst @@ -22,3 +22,5 @@ Magnum Specific Commandments assertIsInstance(A, B). - [M334] Change assertTrue/False(A in/not in B, message) to the more specific assertIn/NotIn(A, B, message) +- [M336] Must use a dict comprehension instead of a dict constructor + with a sequence of key-value pairs. diff --git a/magnum/api/controllers/base.py b/magnum/api/controllers/base.py index 522a478c7f..5b9bc022b6 100644 --- a/magnum/api/controllers/base.py +++ b/magnum/api/controllers/base.py @@ -31,10 +31,10 @@ class APIBase(wtypes.Base): def as_dict(self): """Render this object as a dict of its fields.""" - return dict((k, getattr(self, k)) - for k in self.fields - if hasattr(self, k) and - getattr(self, k) != wsme.Unset) + return {k: getattr(self, k) + for k in self.fields + if hasattr(self, k) and + getattr(self, k) != wsme.Unset} def unset_fields_except(self, except_list=None): """Unset fields so they don't appear in the message body. diff --git a/magnum/cmd/template_manage.py b/magnum/cmd/template_manage.py index a8cb0b5283..23c3dcf205 100644 --- a/magnum/cmd/template_manage.py +++ b/magnum/cmd/template_manage.py @@ -39,7 +39,7 @@ def print_rows(rows): fields.append('path') field_labels.append('Template Path') - formatters = dict((key, operator.itemgetter(key)) for key in fields) + formatters = {key: operator.itemgetter(key) for key in fields} cliutils.print_list(rows, fields, formatters=formatters, diff --git a/magnum/hacking/checks.py b/magnum/hacking/checks.py index b8b6d5c915..67649cd579 100644 --- a/magnum/hacking/checks.py +++ b/magnum/hacking/checks.py @@ -50,6 +50,7 @@ asse_equal_with_is_not_none_re = re.compile( assert_true_isinstance_re = re.compile( r"(.)*assertTrue\(isinstance\((\w|\.|\'|\"|\[|\])+, " "(\w|\.|\'|\"|\[|\])+\)\)") +dict_constructor_with_list_copy_re = re.compile(r".*\bdict\((\[)?(\(|\[)") def check_policy_enforce_decorator(logical_line, @@ -136,6 +137,14 @@ def use_timeutils_utcnow(logical_line, filename): yield (pos, msg % f) +def dict_constructor_with_list_copy(logical_line): + msg = ("M336: Must use a dict comprehension instead of a dict constructor" + " with a sequence of key-value pairs." + ) + if dict_constructor_with_list_copy_re.match(logical_line): + yield (0, msg) + + def factory(register): register(check_policy_enforce_decorator) register(no_mutable_default_args) @@ -145,3 +154,4 @@ def factory(register): register(assert_true_isinstance) register(assert_equal_in) register(use_timeutils_utcnow) + register(dict_constructor_with_list_copy) diff --git a/magnum/objects/base.py b/magnum/objects/base.py index 8f2e81c591..fef7f32b8a 100644 --- a/magnum/objects/base.py +++ b/magnum/objects/base.py @@ -43,9 +43,9 @@ class MagnumObject(ovoo_base.VersionedObject): OBJ_PROJECT_NAMESPACE = 'magnum' def as_dict(self): - return dict((k, getattr(self, k)) - for k in self.fields - if hasattr(self, k)) + return {k: getattr(self, k) + for k in self.fields + if hasattr(self, k)} class MagnumObjectDictCompat(ovoo_base.VersionedObjectDictCompat): diff --git a/magnum/tests/unit/api/utils.py b/magnum/tests/unit/api/utils.py index 57c932dab7..be77c38262 100644 --- a/magnum/tests/unit/api/utils.py +++ b/magnum/tests/unit/api/utils.py @@ -28,7 +28,7 @@ from magnum.tests.unit.db import utils def remove_internal(values, internal): # NOTE(yuriyz): internal attributes should not be posted, except uuid int_attr = [attr.lstrip('/') for attr in internal if attr != '/uuid'] - return dict([(k, v) for (k, v) in values.items() if k not in int_attr]) + return {k: v for (k, v) in values.items() if k not in int_attr} def baymodel_post_data(**kw): diff --git a/magnum/tests/unit/test_hacking.py b/magnum/tests/unit/test_hacking.py index 3a1ebf2abd..0c1a654329 100644 --- a/magnum/tests/unit/test_hacking.py +++ b/magnum/tests/unit/test_hacking.py @@ -204,3 +204,31 @@ class HackingTestCase(base.TestCase): code = "aaa" self._assert_has_no_errors(code, check, filename) + + def test_dict_constructor_with_list_copy(self): + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + " dict([(i, connect_info[i])")))) + + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + " attrs = dict([(k, _from_json(v))")))) + + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + " type_names = dict((value, key) for key, value in")))) + + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + " dict((value, key) for key, value in")))) + + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + "foo(param=dict((k, v) for k, v in bar.items()))")))) + + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + " dict([[i,i] for i in range(3)])")))) + + self.assertEqual(1, len(list(checks.dict_constructor_with_list_copy( + " dd = dict([i,i] for i in range(3))")))) + + self.assertEqual(0, len(list(checks.dict_constructor_with_list_copy( + " create_kwargs = dict(snapshot=snapshot,")))) + + self.assertEqual(0, len(list(checks.dict_constructor_with_list_copy( + " self._render_dict(xml, data_el, data.__dict__)"))))