[Django] #18627: Remove django-core uses of Model pre_init and post_init
Django
-------------------------------------+-------------------------------------
Reporter: jdunck | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
Component: Database | Keywords:
layer (models, ORM) | Has patch: 0
Severity: Normal | Needs tests: 0
Triage Stage: Accepted | Easy pickings: 0
Needs documentation: 0 |
Patch needs improvement: 0 |
UI/UX: 0 |
-------------------------------------+-------------------------------------
GenericForeignKey and ImageField use pre_init and post_init - signals
which are fired on each model construction.
The signal handling code is heavy relative to plain function dispatch, and
is optimized for the zero-handler case. It's unfortunate that Django
ships with these signals being used, as it could instead use an internal
hook for the needed initialization, and this approach would be faster.
I'm working on a patch to switch to a more direct hook (which any field
could use instead of pre/post_init).
See also:
https://groups.google.com/forum/?fromgroups#!search/pre_init$2Fpost_init$20vs.$20performance
/django-developers/GNQsvs8X9Gc/cegjyELUZt8J
(Marking triage accepted since discussed and +1'd by some core folks.)
--
Ticket URL: <https://code.djangoproject.com/ticket/18627>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: jdunck | Owner: nobody
Type: | Status: new
Cleanup/optimization | Version: 1.4
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by jdunck):
* has_patch: 0 => 1
Comment:
Ongoing work is here: https://github.com/jdunck/django/tree/18627_pre_init
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:1>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Version: 1.4
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Changes (by jdunck):
* status: new => assigned
* owner: nobody => jdunck
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:2>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Version: 1.4
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by aaugustin):
A quick summary of the context:
- `GenericForeignKey` uses `pre_init` since day 1, see [bca5327b21]. This
isn't rock-solid — for instance, setting a GFK to an unsaved model
instance doesn't work, even if you save the related model instance first
(there was a ticket about this recently).
- `ImageField` uses `post_init` since #11196, see [d89ba464dd]. That was
three years ago.
The proposed patch re-implements a signal-like pattern, but specific to
model fields. Through their `contribute_to_class` methods, fields can
register additional `pre_init` or `post_init` logic. Unlike the global
`pre_init` and `post_init` signals, this stays within each model's
boundaries, hence the speed optimization.
I'm moderately enthusiastic about this idea. Speeding-up model creation is
a worthy goal, but aren't we trading a short term benefit for more
technical debt? With this change, there will be two competing
`pre/post_init` mechanisms for models. Django (both core and contrib) will
user the faster one.
Do you plan to turn this into a public, documented API? The model API is a
foggy area: the docs encourage people to write their own fields (and we've
repeatedly waved off `JsonField` and `UuidField` for this reason) but they
don't describe very precisely what is kosher and what isn't. If we
introduce this mechanism, people will use it in their own fields. So we're
making this area even more difficult to clean up.
Have you tried to implement the parts of `GenericForeignKey` and
`ImageField` without relying on a model-level hook? Basically, the problem
is that a field must interact with other fields of the same model. It may
be possible to handle this within the field itself. I haven't said it's
easy, but if it was possible, it would be much cleaner, and probably safer
in the long term.
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:3>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Version: 1.4
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
If this would be easy to do in the field itself that would be nice.
Unfortunately any approach where a method is called for each field on
model init is immediately a no-go, as the performance hit is substantial.
What is done in the ticket is basically caching the information which
fields has a method that do something.
If there is a way to do field on-init calls with properties or something
similar, then that would be neat. Still, if I understand correctly one
can't do the post_init stuff at assignment time.
It is true the current approach is a little ugly. One the other hand model
`__init__` speed is important.
With carefully crafted signal receiver caching we can actually get same
performance (if not even little better) than given by this idea, but I
haven't seen too much interest in #16679 from other core devs... In
addition, #16679 or something similar is needed for efficient inherited
signals.
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:4>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Version: 1.4
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I did some work to benchmark the changes. Speed difference between HEAD
and init hooks approach is 1.6x for a project which has both pre and post
init signals from fields (replaced with hooks in init hooks approach). The
receiver caching approach gives pretty much the same result.
To benchmark this I added three new benchmarks to djangobench, available
[https://github.com/akaariai/djangobench/tree/init_signals here].
A little bit cleaned up init hooks approach is available
[https://github.com/akaariai/django/tree/jdunck_preinit here]. The cleanup
is mainly there to remove any possible interference from the testing
hacks. Finally, a rebased version of #16679's patch is available
[https://github.com/akaariai/django/tree/ticket_16679 here].
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:5>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Version: 1.4
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by akaariai):
I wonder if we want to remove the use of signals now that signal caching
is in master. There isn't any compelling speed reason to do so, but init
hooks could be a little cleaner coding style than signals.
Any opinions?
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:6>
Django
-------------------------------------+-------------------------------------
Type: | Status: assigned
Cleanup/optimization | Version: 1.4
Component: Database layer | Resolution:
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
Comment (by jdunck):
If starting from scratch, I would definitely prefer hooks over signals,
but given backwards-compat concerns, I doubt we'll ever fully kill these
signals. As such, I agree, caching probably covers the gain here so we
should kill this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:7>
Django
-------------------------------------+-------------------------------------
Type: | Status: closed
Cleanup/optimization | Version: 1.4
Component: Database layer | Resolution: wontfix
(models, ORM) | Triage Stage: Accepted
Severity: Normal | Needs documentation: 0
Keywords: | Patch needs improvement: 0
Has patch: 1 | UI/UX: 0
Needs tests: 0 |
Easy pickings: 0 |
-------------------------------------+-------------------------------------
* status: assigned => closed
* resolution: => wontfix
Comment:
OK, I think it is time to close this ticket.
--
Ticket URL: <https://code.djangoproject.com/ticket/18627#comment:8>