Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#28860 closed Cleanup/optimization (fixed)

Reduce calls to len()

Reported by: Дилян Палаузов Owned by: nobody
Component: Core (Other) Version: dev
Severity: Normal Keywords:
Cc: Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

if len(x) is the same as if x except when len() is supposed to throw exception when x is not iterable.

len(x) can return either zero or a positive integer, hence if len(x) < 1 is the same as if len(x) == 0 <=> if not x.

Attachments (1)

less-len.patch (18.3 KB ) - added by Дилян Палаузов 7 years ago.

Download all attachments as: .zip

Change History (11)

by Дилян Палаузов, 7 years ago

Attachment: less-len.patch added

comment:1 by Tim Graham, 7 years ago

Component: UncategorizedCore (Other)
Triage Stage: UnreviewedReady for checkin

PR from the patch.

comment:2 by Дилян Палаузов, 7 years ago

Even more places:

diff --git a/django/contrib/admin/filters.py b/django/contrib/admin/filters.py
--- a/django/contrib/admin/filters.py
+++ b/django/contrib/admin/filters.py
@@ -77,7 +77,7 @@ class SimpleListFilter(ListFilter):
         self.lookup_choices = list(lookup_choices)
 
     def has_output(self):
-        return len(self.lookup_choices) > 0
+        return bool(self.lookup_choices)
 
     def value(self):
         """
diff --git a/django/db/backends/oracle/creation.py b/django/db/backends/oracle/creation.py
--- a/django/db/backends/oracle/creation.py
+++ b/django/db/backends/oracle/creation.py
@@ -268,7 +268,7 @@ class DatabaseCreation(BaseDatabaseCreation):
         """
         try:
             # Statement can fail when acceptable_ora_err is not None
-            allow_quiet_fail = acceptable_ora_err is not None and len(acceptable_ora_err) > 0
+            allow_quiet_fail = bool(acceptable_ora_err)
             self._execute_statements(cursor, statements, parameters, verbosity, allow_quiet_fail=allow_quiet_fa
             return True
         except DatabaseError as err:
diff --git a/django/db/migrations/questioner.py b/django/db/migrations/questioner.py
--- a/django/db/migrations/questioner.py
+++ b/django/db/migrations/questioner.py
@@ -85,7 +85,7 @@ class InteractiveMigrationQuestioner(MigrationQuestioner):
         result = input("%s " % question)
         if not result and default is not None:
             return default
-        while len(result) < 1 or result[0].lower() not in "yn":
+        while not result or result[0].lower() not in "yn":
             result = input("Please answer yes or no: ")
         return result[0].lower() == "y"
 
diff --git a/django/db/models/base.py b/django/db/models/base.py
--- a/django/db/models/base.py
+++ b/django/db/models/base.py
@@ -736,7 +736,7 @@ class Model(metaclass=ModelBase):
         """
         using = using or router.db_for_write(self.__class__, instance=self)
         assert not (force_insert and (force_update or update_fields))
-        assert update_fields is None or len(update_fields) > 0
+        assert update_fields is None or update_fields
         cls = origin = self.__class__
         # Skip proxies, but keep the origin as the proxy model.
         if cls._meta.proxy:

comment:3 by Aymeric Augustin, 7 years ago

Some changes in the patch are indeed improvements, but not all of them.

Regarding the comment above, I find that return len(self.lookup_choices) > 0 is much more explicit than return bool(self.lookup_choices) as well as much faster:

>>> import timeit
>>> timeit.timeit('len([]) > 0')
0.0808453639911022
>>> timeit.timeit('bool([])')
0.1313885069976095

comment:4 by Aymeric Augustin, 7 years ago

The performance hit is specific to extra bool calls.

if <something> is faster than if len(<something>).

comment:5 by Tim Graham, 7 years ago

I didn't know that len() is an O(1) operation for many types.

So it looks like for that case, the fastest option would be True if self.lookup_choices else False.

>>> timeit.timeit('True if [1] else False')
0.06975528400289477
>>> timeit.timeit('bool([1])')
0.18175133999829995
>>> timeit.timeit('len([1]) > 0')
0.12322649999987334

Is that optimizing at the expense of readability?

comment:6 by Дилян Палаузов, 7 years ago

len() faster than bool() on lists seems to be deficiency in cpython. I wrote a report https://bugs.python.org/issue32180.

I think for the moment len(self.lookup_choices) > 0 can be left as it is with appropriate comment (e.g. reference to this ticket), so that one does not come to the idea to investigate whether there is place for optimizations on that place.

comment:7 by Aymeric Augustin, 7 years ago

Yes, in my opinion True if ... else False for performance is the worse option there. I agree with Дилян.

comment:8 by Дилян Палаузов, 7 years ago

Another option would be to change the documentation for the return type of has_output: instead of boolean, it returns a value, that can be converted to boolean equivalent to the original return value. In fact, internally has_output is called only on one place.

In db/backends/oracle/creation.py the cast to allow_quiet_fail = bool (acceptable_ora_err) is superfluous, as the called _execute_statements(... allow_quiet_fail=allow_quiet_fail)}}} just calls if not allow_quiet_fail.... Hence calling self._execute_statements(cursor, statements, parameters, verbosity, allow_quiet_fail=acceptable_ora_err) and deleting the local variable allow_quiet_fail is reasonable.

Version 0, edited 7 years ago by Дилян Палаузов (next)

comment:9 by Tim Graham <timograham@…>, 7 years ago

Resolution: fixed
Status: newclosed

In d2afa5eb:

Fixed #28860 -- Removed unnecessary len() calls.

comment:10 by Дилян Палаузов, 7 years ago

My comments for django/db/backends/oracle/creation.py and django/db/backends/postgresql/operations.py weren't tackled.

Note: See TracTickets for help on using tickets.
Back to Top