[Django] #25762: Optimize numberformat.format
Django
--------------------------------------+--------------------
Reporter: jaap3 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
`numberformat.format` is used a lot by Django and in most cases it's
called with a number. Yet the current implementation is pretty much built
for strings.
I've refactored the function to be up to 4x faster on my machine (and in
the worst case it performs roughly the same). Attached is the benchmark
script I've used.
--
Ticket URL: <https://code.djangoproject.com/ticket/25762>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
----------------------------------+----------------------------
Reporter: jaap3 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Unreviewed
Has patch: 1 | Easy pickings: 0
UI/UX: 0 |
Changes (by jaap3):
* Attachment "bench_numberformat.py" added.
Benchmark for numberformat.format
Django
--------------------------------------+------------------------------------
Reporter: jaap3 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: => 0
* needs_docs: => 0
* needs_tests: => 0
* stage: Unreviewed => Accepted
Comment:
I'll mention [https://github.com/django/djangobench/ djangobench] in case
you want to adapt any of your testing scripts so they can live on.
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: jaap3 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by jaap3):
Ah nice, I'll look into adding some benchmarks to djangobench
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: jaap3 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by jaap3):
By the way, while refactoring this code I was wondering if supporting
NUMBER_GROUPING other than 3 is even useful. None of the locales shipped
with Django define anything other than 3, and the other related
configuration options (THOUSAND_SEPARATOR and USE_THOUSAND_SEPARATOR) all
refer to grouping by 3 anyway.
It would also be nice if number formatting on strings was deprecated. It
seems that Django already tries to call it with numbers in the places it's
used, and there's no real check if the strings that are passed into the
function are even number like.
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:3>
Django
--------------------------------------+------------------------------------
Reporter: jaap3 | Owner: nobody
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by timgraham):
* needs_better_patch: 0 => 1
Comment:
Left comments for improvement on the pull request. Maybe you'd like to
raise the other questions on the DevelopersMailingList. I'm not sure about
the answers myself.
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:4>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: tim-
Type: | mccurrach
Cleanup/optimization | Status: assigned
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by tim-mccurrach):
* status: new => assigned
* owner: nobody => tim-mccurrach
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:5>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: tim-
Type: | mccurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by tim-mccurrach):
The approach used in the linked PR
(https://github.com/django/django/pull/5668/files) actually has a problem.
The following approach is used to avoid the rounding behaviour of the
builtin `format` function:
{{{
# If `number` is 10.888 and `decimal_pos` is 2 return
10.88 not 10.89.
format_string = '.%sf' % (decimal_pos + 1)
}}}
and the string is then truncated to `decimal_pos` decimals places.
However, for a number such as 10.999, this will still be rounded 11.00.
This is inconsistent with the behaviour of the rest of the function, which
truncates and doesn't round.
We therefore can't use the bultin `format` to do any kind of decimal
formatting as there doesn't appear (as it currently stands) to be an
option to truncate with it, however the speed advantages it offers can
still be used for the common case of seperating a number into thousands.
Using the test cases in `bench_number_format.py` linked above, an improved
function performs anywhere between 2x and 5x faster for most cases, and
again in the worst case is roughly the same (fluctuating between +2% and
-2% compared with the original function).
I'll add a new PR.
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:6>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: tim-
Type: | mccurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Johannes Hoppe):
* cc: Johannes Hoppe (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Component: Utilities | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Johannes Maron):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Johannes Maron):
* needs_better_patch: 0 => 1
* needs_tests: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Comment (by Jacob Walls):
New [https://github.com/django/django/pull/11844 PR]
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_tests: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Jacob Walls):
* needs_better_patch: 1 => 0
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:12>
Django
-------------------------------------+-------------------------------------
Reporter: Jaap Roes | Owner: Tim
Type: | McCurrach
Cleanup/optimization | Status: assigned
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Mariusz Felisiak):
* needs_better_patch: 0 => 1
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:13>
Django
--------------------------------------+------------------------------------
Reporter: Jaap Roes | Owner: (none)
Type: Cleanup/optimization | Status: new
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Tim McCurrach):
* owner: Tim McCurrach => (none)
* status: assigned => new
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:14>
Django
--------------------------------------+------------------------------------
Reporter: Jaap Roes | Owner: (none)
Component: Utilities | Version: dev
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Easy pickings: 0 | UI/UX: 0
Changes (by Sarah Boyce):
* cc: Sarah Boyce (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:15>
Django
--------------------------------------+------------------------------------
Reporter: Jaap Roes | Owner: (none)
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Changes (by Ülgen Sarıkavak):
* cc: Ülgen Sarıkavak (added)
--
Ticket URL: <https://code.djangoproject.com/ticket/25762#comment:16>