Opened 3 years ago

Last modified 3 years ago

#25762 new Cleanup/optimization

Optimize numberformat.format

Reported by: Jaap Roes Owned by: nobody
Component: Utilities Version: master
Severity: Normal Keywords:
Cc: Triage Stage: Accepted
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: yes
Easy pickings: no UI/UX: no

Description

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.

Attachments (1)

bench_numberformat.py (2.6 KB) - added by Jaap Roes 3 years ago.
Benchmark for numberformat.format

Download all attachments as: .zip

Change History (5)

Changed 3 years ago by Jaap Roes

Attachment: bench_numberformat.py added

Benchmark for numberformat.format

comment:1 Changed 3 years ago by Tim Graham

Triage Stage: UnreviewedAccepted

I'll mention djangobench in case you want to adapt any of your testing scripts so they can live on.

comment:2 Changed 3 years ago by Jaap Roes

Ah nice, I'll look into adding some benchmarks to djangobench

comment:3 Changed 3 years ago by Jaap Roes

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.

comment:4 Changed 3 years ago by Tim Graham

Patch needs improvement: set

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.

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