Opened 10 months ago

Closed 9 months ago

#33355 closed Cleanup/optimization (fixed)

Optimize SQLite backend connection functions

Reported by: Adam Johnson Owned by: Adam Johnson
Component: Database layer (models, ORM) Version: dev
Severity: Normal Keywords:
Cc: Nick Pope, Carlton Gibson 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

The SQLite backend registers a bunch of data conversion functions when creating a new connection, with conn.create_function. These functions use the @none_guard decorator to check for Nones in arguments:

def none_guard(func):
    """
    Decorator that returns None if any of the arguments to the decorated
    function are None. Many SQL functions return NULL if any of their arguments
    are NULL. This decorator simplifies the implementation of this for the
    custom functions registered below.
    """
    @functools.wraps(func)
    def wrapper(*args, **kwargs):
        return None if None in args else func(*args, **kwargs)
    return wrapper

This decorator is wasteful. At call time, it makes Python coerce arguments to *args and kwargs and back , and as a decorator it forces two layers of functions where one would do.

We can save this time by explicitly "inlining" the None checks. This saves a small amount of time per call, but considering that these functions can be called thousands of times in a single query, it adds up fast. (...perhaps millions on larger data sets or suboptimal queries!)

Additionally, time and memory is wasted on every new SQLite connection applying the decorator to create new copies of the same function. (And list_aggregate() creates the exact same classes each time.) Considering that Django creates a new connection per request by default (and per test), this adds up.

A small benchmark with one of the shorter functions (Python 3.10.0):

In [1]: import functools

In [2]: def none_guard(func):
   ...:     """
   ...:     Decorator that returns None if any of the arguments to the decorated
   ...:     function are None. Many SQL functions return NULL if any of their arguments
   ...:     are NULL. This decorator simplifies the implementation of this for the
   ...:     custom functions registered below.
   ...:     """
   ...:     @functools.wraps(func)
   ...:     def wrapper(*args, **kwargs):
   ...:         return None if None in args else func(*args, **kwargs)
   ...:     return wrapper
   ...:

In [3]: @none_guard
   ...: def _sqlite_rpad(text, length, fill_text):
   ...:     return (text + fill_text * length)[:length]
   ...:

In [4]: def _sqlite_rpad_inlined(text, length, fill_text):
   ...:     if text is None or length is None or fill_text is None:
   ...:         return None
   ...:     return (text + fill_text * length)[:length]
   ...:

In [5]: %timeit _sqlite_rpad("Hello world", 20, " ")
534 ns ± 30.4 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

In [6]: %timeit _sqlite_rpad_inlined("Hello world", 20, " ")
328 ns ± 26.5 ns per loop (mean ± std. dev. of 7 runs, 1000000 loops each)

~40% reduction.

Change History (17)

comment:1 Changed 10 months ago by Mariusz Felisiak

Cc: Nick Pope added

This would force us to create small wrappers for many (~20) functions from the math module.

comment:2 Changed 10 months ago by Adam Johnson

Has patch: set

Yes. Luckily I've got the patience to do that.

comment:3 Changed 10 months ago by Carlton Gibson

Cc: Carlton Gibson added

comment:4 Changed 10 months ago by Mariusz Felisiak

Owner: changed from nobody to Adam Johnson
Triage Stage: UnreviewedAccepted

comment:5 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In c66ecc5:

Refs #33355 -- Moved Trunc() assertions for invalid arguments and ISO 8601 week to separate tests.

comment:6 Changed 10 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 5111b63:

Refs #33355 -- Fixed Trunc() with years < 1000 on SQLite.

Thanks to Nick Pope for spotting the bug in Code Review.

Co-Authored-By: Nick Pope <nick@…>

comment:7 Changed 9 months ago by Nick Pope

Triage Stage: AcceptedReady for checkin

Thanks Adam. I think this is ready.

comment:8 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In a8fa3e5c:

Refs #33355 -- Added missing tests for database functions and expression on null values.

comment:9 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In c4328c2f:

Refs #33355 -- Optimized Trunc() on SQLite by using f-strings.

Co-Authored-By: Nick Pope <nick@…>

comment:10 Changed 9 months ago by Mariusz Felisiak

Patch needs improvement: set
Triage Stage: Ready for checkinAccepted

comment:11 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In fa4b2c15:

Refs #33355 -- Optimized LPad() database function on SQLite.

Co-Authored-By: Nick Pope <nick@…>

comment:12 Changed 9 months ago by Mariusz Felisiak

Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:13 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 2d991ff6:

Refs #33355 -- Moved SQLite functions to separate module.

Co-Authored-By: Nick Pope <nick@…>

comment:14 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In deec15a9:

Refs #33355 -- Made trunc functions raise ValueError on invalid lookups on SQLite.

Co-Authored-By: Nick Pope <nick@…>

comment:15 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In ec7554f1:

Refs #33355 -- Removed @none_guard from SQLite functions.

Co-Authored-By: Nick Pope <nick@…>

comment:16 Changed 9 months ago by Mariusz Felisiak <felisiak.mariusz@…>

In 5f6a727a:

Refs #33355 -- Constructed SQLite list aggregate types once.

comment:17 Changed 9 months ago by Mariusz Felisiak

Resolution: fixed
Status: assignedclosed
Note: See TracTickets for help on using tickets.
Back to Top