[Django] #24591: Copy ModelState.fields and ModelState.managers instead of clone
Django
--------------------------------------+--------------------
Reporter: knbk | Owner: knbk
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Severity: Normal | Keywords:
Triage Stage: Unreviewed | Has patch: 1
Easy pickings: 0 | UI/UX: 0
--------------------------------------+--------------------
Fields in a`ModelState` are guaranteed to not be bound to a model, and are
deconstructed and reconstructed in `ModelState.from_model()`. They are
also properly deconstructed and reconstructed when rendering a model.
Furthermore, the fields themselves must not change during the lifetime of
a `ModelState` object, though you may freely add, remove and replace
fields (1). They should be considered immutable within model states.
I think the current implementation of `ModelState.clone()` is too
conservative in this regard. Both the input (in `ModelState.from_model()`)
and the output (in `ModelState.render()`) do a full de- and reconstruct.
The only change in between is the addition and removal of fields from the
list. `ModelState.clone()` can do a simple `list(self.fields)` and still
maintain the correct behaviour. The same holds for `ModelState.managers`.
This change saves about 75% in rendering time, and 66% in memory usage in
my benchmarks.
If you want to safeguard against accidental changes that can affect other
model states, you can use `copy.deepcopy()`, though at a performance cost.
Other options are to somehow shield against accidental changes by
generating an immutable field class, or by saving the deconstructed field
and constructing the fields on demand.
As this is ''the'' most performance-critical part of migrations -- save
the actual database manipulation -- I suggest we take a careful look into
this. The current implementation with `deconstruct()` is far from the
ideal solution.
(1)
https://github.com/django/django/blob/master/django/db/migrations/state.py#L294
--
Ticket URL: <https://code.djangoproject.com/ticket/24591>
Django <https://code.djangoproject.com/>
The Web framework for perfectionists with deadlines.
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: new
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage:
| Unreviewed
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knbk):
* needs_docs: => 0
* needs_better_patch: => 0
* needs_tests: => 0
Comment:
PR: https://github.com/django/django/pull/4460
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:1>
Django
--------------------------------------+------------------------------------
Reporter: knbk | Owner: knbk
Type: Cleanup/optimization | Status: new
Component: Migrations | Version: master
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by MarkusH):
* stage: Unreviewed => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:2>
Django
--------------------------------------+------------------------------------
Reporter: knbk | Owner: knbk
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by knbk):
* status: new => assigned
Comment:
The last commit in !4460 stores the deconstructed state when initializing
the wrapper. I think that's the most comprehensive protection we can
offer, without a noticeable performance cost. We might issue a warning
instead of raising an exception in `__setattr__`: it is no longer an error
to change the field, it is simply useless.
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:3>
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by timgraham):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:4>
Django
--------------------------------------+------------------------------------
Reporter: knbk | Owner: knbk
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Easy pickings: 0 | UI/UX: 0
Changes (by charettes):
* needs_better_patch: 0 => 1
* stage: Ready for checkin => Accepted
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:5>
Django
--------------------------------------+------------------------------------
Reporter: knbk | Owner: knbk
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 1
Easy pickings: 0 | UI/UX: 0
Changes (by MarkusH):
* needs_docs: 0 => 1
Comment:
Checking "Needs documentation" because the operations documentation needs
a warning about not modifying fields and possible breaking change for 3rd
party operations in the release notes.
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:6>
Django
--------------------------------------+------------------------------------
Reporter: knbk | Owner: knbk
Type: Cleanup/optimization | Status: assigned
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Accepted
Needs tests: 0 | Patch needs improvement: 0
Easy pickings: 0 | UI/UX: 0
Changes (by knbk):
* needs_better_patch: 1 => 0
* needs_docs: 1 => 0
Comment:
A more conservative backport for 1.8:
https://github.com/django/django/pull/4533
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:7>
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Needs tests: 0 | Patch needs improvement: 0
Changes (by MarkusH):
* stage: Accepted => Ready for checkin
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:8>
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: assigned
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution:
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by Tim Graham <timograham@…>):
In [changeset:"516907540b81c009b74d29b2fa36319b47528b49" 51690754]:
{{{
#!CommitTicketReference repository=""
revision="516907540b81c009b74d29b2fa36319b47528b49"
[1.8.x] Refs #24591 -- Optimized cloning of ModelState objects.
Changed ModelState.clone() to create a deepcopy of self.fields.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:9>
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Changes (by Tim Graham <timograham@…>):
* status: assigned => closed
* resolution: => fixed
Comment:
In [changeset:"1a1f16d67da7080241ad9f6b4325f22364ac57ba" 1a1f16d6]:
{{{
#!CommitTicketReference repository=""
revision="1a1f16d67da7080241ad9f6b4325f22364ac57ba"
Fixed #24591 -- Optimized cloning of ModelState objects.
Changed ModelState.clone() to create a shallow copy of self.fields
and self.managers.
}}}
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:10>
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by dalore):
I know this has been marked fixed but how is this fixed? I followed
another ticket to here in which was about the migrations being slow, well
this is what I get after upgrading Django from 1.6 to 1.8 when running
test (To see if everything works):
Running migrations:
Rendering model states...
DONE (1410.571s)
Then it goes on and runs each migration taking 1 - 60 per migrations. How
is 20+ minutes acceptable? This is the time it takes before it even starts
applying the new initial migrations.
This is an old project that started in the early django days, even Andrew
Godwin himself worked on this project for a bit.
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:11>
Django
-------------------------------------+-------------------------------------
Reporter: knbk | Owner: knbk
Type: | Status: closed
Cleanup/optimization |
Component: Migrations | Version: master
Severity: Normal | Resolution: fixed
Keywords: | Triage Stage: Ready for
| checkin
Has patch: 1 | Needs documentation: 0
Needs tests: 0 | Patch needs improvement: 0
Comment (by timgraham):
It's an ongoing project to improve the speed of migrations. Django 1.9 has
more improvements, please test it if you are able. Feel free to create a
ticket with details and profiling output to help us out. Unfortunately, I
don't think Django 1.8 is likely to get more improvements at this point.
--
Ticket URL: <https://code.djangoproject.com/ticket/24591#comment:12>