Opened 2 weeks ago

Closed 10 days ago

Last modified 10 days ago

#28860 closed Cleanup/optimization (fixed)

Reduce calls to len()

Reported by: Дилян Палаузов Owned by: nobody
Component: Core (Other) Version: master
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 Дилян Палаузов 2 weeks ago.

Download all attachments as: .zip

Change History (11)

Changed 2 weeks ago by Дилян Палаузов

Attachment: less-len.patch added

comment:1 Changed 2 weeks ago by Tim Graham

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

PR from the patch.

comment:2 Changed 2 weeks ago by Дилян Палаузов

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 Changed 2 weeks ago by Aymeric Augustin

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 Changed 2 weeks ago by Aymeric Augustin

The performance hit is specific to extra bool calls.

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

comment:5 Changed 2 weeks ago by Tim Graham

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 Changed 2 weeks ago by Дилян Палаузов

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 Changed 2 weeks ago by Aymeric Augustin

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

comment:8 Changed 2 weeks ago by Дилян Палаузов

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.

In django/db/backends/postgresql/operations.py the code can be shortened to column_name = sequence_info['column'] or 'id'.

Last edited 11 days ago by Дилян Палаузов (previous) (diff)

comment:9 Changed 10 days ago by Tim Graham <timograham@…>

Resolution: fixed
Status: newclosed

In d2afa5eb:

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

comment:10 Changed 10 days ago by Дилян Палаузов

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