[Django] #28860: Reduce calls to len()
Django
------------------------------------------------+------------------------
Reporter: Дилян Палаузов | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Uncategorized | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Needs documentation: 0 | Needs tests: 0
Patch needs improvement: 0 | Easy pickings: 0
UI/UX: 0 |
------------------------------------------------+------------------------
{{{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}}}.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Uncategorized | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Дилян Палаузов):
* Attachment "less-len.patch" added.
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham):
* component: Uncategorized => Core (Other)
* stage: Unreviewed => Ready for checkin
Comment:
[https://github.com/django/django/pull/9398 PR] from the patch.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:1>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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:
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:2>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:3>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
The performance hit is specific to extra `bool` calls.
`if <something>` is faster than `if len(<something>)`.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:4>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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?
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:5>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:6>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Aymeric Augustin):
Yes, in my opinion `True if ... else False` for performance is the worse
option there. I agree with Дилян.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:7>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (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.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:8>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim Graham <timograham@…>):
* status: new => closed
* resolution: => fixed
Comment:
In [changeset:"d2afa5eb2308e672b6313876856e32e2561b90f3" d2afa5eb]:
{{{
#!CommitTicketReference repository=""
revision="d2afa5eb2308e672b6313876856e32e2561b90f3"
Fixed #28860 -- Removed unnecessary len() calls.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:9>
Django
Reporter: Дилян Палаузов | Owner: nobody
Cleanup/optimization |
Component: Core (Other) | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Дилян Палаузов):
My comments for django/db/backends/oracle/creation.py and
django/db/backends/postgresql/operations.py weren't tackled.
--
Ticket URL: <https://code.djangoproject.com/ticket/28860#comment:10>