[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>