[Django] #36815: Avoid unnecessary prepare_value calls when inserting db_defaults

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Type:
| Cleanup/optimization
Status: new | Component: Database
| layer (models, ORM)
Version: 6.0 | Severity: Normal
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
In `SQLInsertCompiler.as_sql`, it looks like `prepare_value` is
unnecessarily called for each `DatabaseDefault` value on every inserted
object. These calls are quite expensive (as they enter the `if
hasattr(value, "resolve_expression")` branch) and waste a lot of time when
using `bulk_create`.

I've written a potential patch and a simple benchmark:
https://github.com/adamsol/django/commit/d6f275c8960988c92a9da5eb323b6bd6fc7717cb.
The benchmark runs 2x faster with the fix, even though there's only one
field with `db_default` in that model. The difference may become larger
with more fields.
--
Ticket URL: <https://code.djangoproject.com/ticket/36815>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------

Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rish):

Can you please share the benchmark results?
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:1>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Adam Sołtysik):

`python runtests.py bulk_create -k perf`

Lowest times from several runs:
* 0.117 with the fix
* 0.225 after reverting the fix
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:2>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rish):

Moreover it seems your potential fix is breaking this testcase:

{{{
FAIL: test_crafted_xml_performance
(serializers.test_deserialization.TestDeserializer.test_crafted_xml_performance)
[constant depth, varying length]
The time to process invalid inputs is not quadratic.
----------------------------------------------------------------------
Traceback (most recent call last):
File "/usr/lib64/python3.14/unittest/case.py", line 58, in
testPartExecutor
yield
File "/usr/lib64/python3.14/unittest/case.py", line 565, in subTest
yield
File
"/home/rishu/Developer/OSS/django/tests/serializers/test_deserialization.py",
line 182, in assertFactor
self.assertLessEqual(sum(factors) / len(factors), factor)
^^^^^^^
File "/usr/lib64/python3.14/unittest/case.py", line 1303, in
assertLessEqual
self.fail(self._formatMessage(msg, standardMsg))
^^^^^^^^^^^
File "/usr/lib64/python3.14/unittest/case.py", line 750, in fail
raise self.failureException(msg)
^^^^^^^^^^^^^^^
AssertionError: 2.2543277454075485 not less than or equal to 2

----------------------------------------------------------------------
Ran 18875 tests in 103.044s

FAILED (failures=1, skipped=1458, expected failures=5)
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database
}}}

Otherwise it works fine:

{{{
...s..................s....................................................................................
----------------------------------------------------------------------
Ran 18875 tests in 106.334s

OK (skipped=1458, expected failures=5)
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:3>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Adam Sołtysik):

That test can apparently sometimes fail randomly, see the comment:

{{{
# Assert based on the average factor to reduce test flakiness.
self.assertLessEqual(sum(factors) / len(factors), factor)
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:4>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Rish):

Thanks. Also, can you please help me understand why this fix would work
here specifically? I thought about fixing this in the `prepare_value`
method, somewhat like this:

`django/db/models/sql/compiler.py:1722`
{{{
def prepare_value(self, field, value):
from django.db.models.expressions import DatabaseDefault
"""
Prepare a value to be used in a query by resolving it if it is an
expression and otherwise calling the field's get_db_prep_save().
"""


if hasattr(value, "resolve_expression"):

if not isinstance(value, DatabaseDefault):
return value

value = value.resolve_expression(
self.query, allow_joins=False, for_save=True
)
}}}

And an absurd number of testcases failed due to this. Maybe this might be
a problem with my approach of generalization or this optimization is
really specific to the case here. Either way, adding that context would he
helpful.
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:5>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Adam Sołtysik):

First of all, the condition has the wrong logic in this variant, it should
be without negation. But apart from that, the `field_prepare` function
(and so `prepare_value`) is called on `DatabaseDefault` values also later
in the code, for databases that don't satisfy
`supports_default_keyword_in_bulk_insert` (Oracle), so this approach
probably wouldn't work there.
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:6>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |
Component: Database layer | Version: 6.0
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Nilesh Pahari):

* cc: Nilesh Pahari (added)

--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:7>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner: (none)
Type: | Status: new
Cleanup/optimization |

Component: Database layer | Version: dev

(models, ORM) |
Severity: Normal | Resolution:

Keywords: | Triage Stage: Accepted

Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by Jacob Walls):

* stage: Unreviewed => Accepted
* version: 6.0 => dev

Comment:

Thanks for the benchmark, this seems reasonable. Would you like to submit
a PR? We can discuss there if we should at least call `field_prepare()`
once instead of zero times per db-default-field, but certainly not N
times.

(Btw, regarding the flaky test, it's a known issue and is being removed in
an unrelated PR.)
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:8>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:

Type: | YashRaj1506
Cleanup/optimization | Status: assigned

Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Changes (by YashRaj1506):

* owner: (none) => YashRaj1506
* status: new => assigned

--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:9>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by YashRaj1506):

Okay so i went through this, and i actually didnt notice a pr was
attatched but my solution is also exactly what Adam came up with. "
Ignoring the `field_prepare(field_pre_save(obj))` from happening if it is
a `DatabaseDefault`object and just passing the value we get from
`field_pre_save(obj)` so that we dont break any operation which is
happening after this point.

I borrowed the test from adams pr and everything passed nicely, also
improving the benchmark.

without new code


{{{

(djangodev) yash@pop-os:~/Desktop/36815/django/tests$ python runtests.py
bulk_create
Testing against Django installed in
'/home/yash/Desktop/36815/django/django' with up to 16 processes
Found 60 test(s).
Creating test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
System check identified no issues (0 silenced).
...0.217
.............s.........................s...s...s......sss
----------------------------------------------------------------------
Ran 60 tests in 0.368s

OK (skipped=7)

Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
}}}

with imrpovements:

{{{
(djangodev) yash@pop-os:~/Desktop/36815/django/tests$ python runtests.py
bulk_create
Testing against Django installed in
'/home/yash/Desktop/36815/django/django' with up to 16 processes
Found 60 test(s).
Creating test database for alias 'default'...
Cloning test database for alias 'default'...
Cloning test database for alias 'default'...
System check identified no issues (0 silenced).
...0.117
.............s.........................s...s...s......sss
----------------------------------------------------------------------
Ran 60 tests in 0.275s

OK (skipped=7)

Destroying test database for alias 'default'...
Destroying test database for alias 'default'...
Destroying test database for alias 'default'..
}}}

--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:10>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------
Comment (by YashRaj1506):

Replying to [comment:8 Jacob Walls]:

> Thanks for the benchmark, this seems reasonable. Would you like to
submit a PR? We can discuss there if we should at least call
`field_prepare()` once instead of zero times per db-default-field, but
certainly not N times.
>
> (Btw, regarding the flaky test, it's a known issue and is being removed
in an unrelated PR.)

Also i want to ask, why are we tyring to run field prepare here? i didnt
understand that part.
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:11>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by Adam Sołtysik):

As far as I'm concerned, feel free to submit a PR. I didn't do it myself
mainly because with things like CLA it's a little bit too much bureaucracy
for me. Also, my benchmark was obviously not designed to end up in the
actual test suite. I guess that a proper regression test should be
written, although I'm not sure what's the policy for testing against
executing unnecessary code.
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:12>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 0 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
-------------------------------------+-------------------------------------

Comment (by YashRaj1506):

Yeah sure its more of a benchmark, i will write the proper regression test
here and make the pr.
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:13>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
Severity: Normal | Resolution:
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 YashRaj1506):

* has_patch: 0 => 1

--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:14>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506
Cleanup/optimization | Status: assigned
Component: Database layer | Version: dev
(models, ORM) |
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 Jacob Walls):

* stage: Accepted => Ready for checkin

--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:15>

[Django]

#36815: Avoid unnecessary prepare_value calls when inserting db_defaults
-------------------------------------+-------------------------------------
Reporter: Adam Sołtysik | Owner:
Type: | YashRaj1506

Cleanup/optimization | Status: closed

Component: Database layer | Version: dev
(models, ORM) |

Severity: Normal | Resolution: fixed

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 Jacob Walls <jacobtylerwalls@…>):

* resolution: => fixed
* status: assigned => closed

Comment:

In [changeset:"9247410b4ba3b1f855567b8d84422d36345c690a" 9247410b]:
{{{#!CommitTicketReference repository=""
revision="9247410b4ba3b1f855567b8d84422d36345c690a"
Fixed #36815 -- Optimized insertion of db_default fields in bulk_create().

Thanks Adam Sołtysik for the implementation idea.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/36815#comment:16>