[Soc2014] The new Options API #2894

Closed
wants to merge 355 commits into
from

Conversation

Projects
None yet
9 participants
@PirosB3
Contributor

PirosB3 commented Jul 8, 2014

Main API

Official Wiki page: https://code.djangoproject.com/wiki/new_meta_api

get_fields(m2m, data, related_m2m, related_objects, virtual,
include_parents, include_non_concrete, include_hidden, include_proxy) -> (field, field, ...)

get_field(field_name, m2m, data, related_m2m, related_objects, virtual) -> field

field_names -> (field_name, field_name, ..)

Cached properties for fast access

fields -> (field, field, ..)
concrete_fields -> (field, field, ...)
local_concrete_fields -> (field, field, ...)
field_names (field_name, field_name, ..)

Benchmarks

(compared to master)

Benchmark Name Control Experiment Diff
url_resolve_flat_i18n_off 0.3224664569 0.3128736854 0.0095927715 (2.97% faster)
url_resolve_flat 0.2262115002 0.2201457143 0.0060657859 (2.68% faster)
query_prefetch_related 0.1185187101 0.1131614804 0.0053572297 (4.52% faster)
query_all 0.0260368466 0.0244047880 0.0016320586 (6.27% faster)
query_select_related 0.0590585470 0.0582756639 0.0007828832 (1.33% faster)
query_raw 0.0150311589 0.0149269700 0.0001041889 (0.69% faster)
query_distinct 0.0003427148 0.0003322124 0.0000105023 (3.06% faster)
url_resolve 0.0174066782 0.0174011946 0.0000054836 (0.03% faster)
template_render_simple 0.0001164556 0.0001118183 0.0000046372 (3.98% faster)
query_delete_related 0.0003562689 0.0003521204 0.0000041485 (1.16% faster)
form_create 0.0001154184 0.0001116633 0.0000037551 (3.25% faster)
model_creation 0.0002620935 0.0002592921 0.0000028014 (1.07% faster)
query_iterator 0.0002910018 0.0002886057 0.0000023961 (0.82% faster)
form_clean 0.0000283957 0.0000273466 0.0000010490 (3.69% faster)
url_resolve_nested 0.0001253605 0.0001250029 0.0000003576 (0.29% faster)
query_delete 0.0002635837 0.0002634764 0.0000001073 (0.04% faster)
raw_sql 0.0000513077 0.0000515938 -0.0000002861 (0.56% slower)
template_render 0.0123201489 0.0123205304 -0.0000003815 (0.00% slower)
query_none 0.0001389146 0.0001396894 -0.0000007749 (0.56% slower)
query_all_multifield 0.0442516685 0.0442527294 -0.0000010610 (0.00% slower)
multi_value_dict 0.0001797199 0.0001827717 -0.0000030518 (1.70% slower)
query_count 0.0002346754 0.0002384424 -0.0000037670 (1.61% slower)
locale_from_request 0.0008673787 0.0008715868 -0.0000042081 (0.49% slower)
query_exclude 0.0005133271 0.0005178809 -0.0000045538 (0.89% slower)
query_latest 0.0003925323 0.0003977776 -0.0000052452 (1.34% slower)
model_delete 0.0004462004 0.0004532099 -0.0000070095 (1.57% slower)
query_order_by 0.0003699422 0.0003770232 -0.0000070810 (1.91% slower)
query_aggregate 0.0003185034 0.0003258467 -0.0000073433 (2.31% slower)
default_middleware 0.0004270792 0.0004346967 -0.0000076175 (1.78% slower)
query_filter 0.0004306793 0.0004388094 -0.0000081301 (1.89% slower)
query_update 0.0002333641 0.0002430677 -0.0000097036 (4.16% slower)
query_in_bulk 0.0004901409 0.0005011201 -0.0000109792 (2.24% slower)
query_dates 0.0005465746 0.0005576730 -0.0000110984 (2.03% slower)
query_annotate 0.0006684303 0.0006795764 -0.0000111461 (1.67% slower)
query_values 0.0003065467 0.0003197312 -0.0000131845 (4.30% slower)
query_complex_filter 0.0001333117 0.0001471758 -0.0000138640 (10.40% slower)
query_values_list 0.0002993703 0.0003140211 -0.0000146508 (4.89% slower)
template_compilation 0.0005861163 0.0006127477 -0.0000266314 (4.54% slower)
query_get_or_create 0.0013090134 0.0013397813 -0.0000307679 (2.35% slower)
l10n_render 0.0063407421 0.0063731670 -0.0000324249 (0.51% slower)
query_exists 0.0009287477 0.0009753466 -0.0000465989 (5.02% slower)
qs_filter_chaining 0.0008348227 0.0009381652 -0.0001033425 (12.38% slower)
query_raw_deferred 0.0182870388 0.0186469555 -0.0003599167 (1.97% slower)
model_save_new 0.0142104745 0.0145826817 -0.0003722072 (2.62% slower)
model_save_existing 0.0140855193 0.0145498991 -0.0004643798 (3.30% slower)
query_get 0.0273179531 0.0280359745 -0.0007180214 (2.63% slower)

Sum of all benchmarks (bigger is better): 0.0212707281

PirosB3 added some commits Jun 16, 2014

@funkybob

This comment has been minimized.

Show comment
Hide comment
@funkybob

funkybob Jun 22, 2014

Shouldn't this be using six.iteritems() ?

Also, is this measurably faster than a simple dict comprehension? It's certainly less readable :)

Shouldn't this be using six.iteritems() ?

Also, is this measurably faster than a simple dict comprehension? It's certainly less readable :)

This comment has been minimized.

Show comment
Hide comment
@funkybob

funkybob Jun 22, 2014

Further, this dict comprehension avoids the need for a lambda [and thus a stack frame] as well as the call to map... worth trying.

return {
    field: fn
    for fn, field in six.iteritems(self.get_new_fields(types=types, recursive=True))
}

Further, this dict comprehension avoids the need for a lambda [and thus a stack frame] as well as the call to map... worth trying.

return {
    field: fn
    for fn, field in six.iteritems(self.get_new_fields(types=types, recursive=True))
}
docs/releases/1.8.txt
@@ -23,7 +23,13 @@ Like Django 1.7, Django 1.8 requires Python 2.7 or above, though we
What's new in Django 1.8
========================
-...
+* Added the new Model._meta API in the :class:`~django.db.models.options.Options`

This comment has been minimized.

@timgraham

timgraham Aug 5, 2014

Member

please add a heading rather than using a bulleted list for major features as in 1.7 release notes

@timgraham

timgraham Aug 5, 2014

Member

please add a heading rather than using a bulleted list for major features as in 1.7 release notes

docs/releases/1.8.txt
@@ -428,6 +435,13 @@ Miscellaneous
Features deprecated in 1.8
==========================
+``django.db.models.options.Options``

This comment has been minimized.

@timgraham

timgraham Aug 5, 2014

Member

Selected methods in django.db.models.options.Options

@timgraham

timgraham Aug 5, 2014

Member

Selected methods in django.db.models.options.Options

docs/releases/1.8.txt
@@ -428,6 +435,13 @@ Miscellaneous
Features deprecated in 1.8
==========================
+``django.db.models.options.Options``
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

This comment has been minimized.

@timgraham

timgraham Aug 5, 2014

Member

fix underline length and add newline after

@timgraham

timgraham Aug 5, 2014

Member

fix underline length and add newline after

docs/releases/1.8.txt
+~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
+A series of methods in :class:`~django.db.models.options.Options` have been
+deprecated in favor of newer ones. Each deprecated method is specified in
+:ref:`the deprecation timeline <deprecation-removed-in-2.0>`, it will be backwards-compatible till

This comment has been minimized.

@timgraham

timgraham Aug 5, 2014

Member

Please list the methods here.
"They will be removed in Django 2.0." (don't need to mention backwards compatible/raise warning as that's the case for all deprecated features)

@timgraham

timgraham Aug 5, 2014

Member

Please list the methods here.
"They will be removed in Django 2.0." (don't need to mention backwards compatible/raise warning as that's the case for all deprecated features)

PirosB3 added some commits Aug 5, 2014

Merge branch 'master' of https://github.com/django/django into soc201…
…4_meta_refactor_upgrade_flags_get_field

Conflicts:
	django/db/migrations/operations/models.py
	docs/internals/deprecation.txt
django/apps/registry.py
+ be used in tests or when a new model is added.
+ """
+ if not self.ready:
+ raise AppRegistryNotReady("App registry isn't ready yet. relation tree \

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

don't use backslash for line continuation, see the style used in the rest of this file.

@timgraham

timgraham Aug 7, 2014

Member

don't use backslash for line continuation, see the style used in the rest of this file.

django/contrib/admin/options.py
@@ -401,7 +401,7 @@ def lookup_allowed(self, lookup, value):
rel_name = None
for part in parts[:-1]:
try:
- field, _, _, _ = model._meta.get_field_by_name(part)
+ field = model._meta.get_field(part, related_objects=True, related_m2m=True, virtual=True)

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

You removed virtual=True from the one place I commented, but not everywhere else you used the same pattern. Anyway, did you decide about using a shortcut function?

@timgraham

timgraham Aug 7, 2014

Member

You removed virtual=True from the one place I commented, but not everywhere else you used the same pattern. Anyway, did you decide about using a shortcut function?

This comment has been minimized.

@PirosB3

PirosB3 Aug 8, 2014

Contributor

OK, so...
@timgraham @freakboy3742

There are 3 used combinations of get_field() in the entire Django codebase

get_field(data=True, m2m=True) > 60% of the occurrences
get_field(data=True, m2m=True, related_objects=True, related_m2m=True, virtual=True) > 39.9% of the occurrences
get_field(data=False, m2m=True) once only! (0.01% of occurrences).

My proposal

go back to the old get_field(field_name, include_related=False) and handle the occurance that happens only once indipendently.
Benefits:

  • better and simpler API
  • better memory management (caching)

What do you think?

@PirosB3

PirosB3 Aug 8, 2014

Contributor

OK, so...
@timgraham @freakboy3742

There are 3 used combinations of get_field() in the entire Django codebase

get_field(data=True, m2m=True) > 60% of the occurrences
get_field(data=True, m2m=True, related_objects=True, related_m2m=True, virtual=True) > 39.9% of the occurrences
get_field(data=False, m2m=True) once only! (0.01% of occurrences).

My proposal

go back to the old get_field(field_name, include_related=False) and handle the occurance that happens only once indipendently.
Benefits:

  • better and simpler API
  • better memory management (caching)

What do you think?

@@ -62,10 +62,9 @@ def add_srs_entry(srs, auth_name='EPSG', auth_srid=None, ref_sys_name=None,
}
# Backend-specific fields for the SpatialRefSys model.
- srs_field_names = SpatialRefSys._meta.get_all_field_names()

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

I would have kept the srs_field_names variable.

@timgraham

timgraham Aug 7, 2014

Member

I would have kept the srs_field_names variable.

This comment has been minimized.

@PirosB3

PirosB3 Aug 8, 2014

Contributor

done

@PirosB3

PirosB3 Aug 8, 2014

Contributor

done

django/db/models/deletion.py
@@ -172,7 +183,7 @@ def collect(self, objs, source=None, nullable=False, collect_related=True,
model = new_objs[0].__class__
# Recursively collect concrete model's parent models, but not their
- # related objects. These will be found by meta.get_all_related_objects()
+ # related objects. These will be found by meta.get_new_fields

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

get_new_fields isn't a thing I think

@timgraham

timgraham Aug 7, 2014

Member

get_new_fields isn't a thing I think

@@ -30,9 +30,14 @@ def __init__(self, ct_field="content_type", fk_field="object_id", for_concrete_m
def contribute_to_class(self, cls, name, **kwargs):
self.name = name
+
+ # Set model to cls (as it is not associated to anything)

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

This comment doesn't really do anything to clarify things for me (and I see the same pattern elsewhere uncommented).

@timgraham

timgraham Aug 7, 2014

Member

This comment doesn't really do anything to clarify things for me (and I see the same pattern elsewhere uncommented).

django/db/models/options.py
- # of the chain to the ancestor is that parent
- # links
- return self.parents[parent] or parent_link
+ return list(map(self._map_model, self.get_fields(data=False, related_m2m=True)))

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

I would use a list comprehension rather than list(map())

@timgraham

timgraham Aug 7, 2014

Member

I would use a list comprehension rather than list(map())

django/db/models/options.py
+ return map(self._map_model, self.get_all_related_objects(
+ local_only=local_only,
+ include_hidden=include_hidden,
+ include_proxy_eq=include_proxy_eq

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

add trailing comma

@timgraham

timgraham Aug 7, 2014

Member

add trailing comma

django/db/models/options.py
- % (self.object_name, name))
+ return self._map_model_details(self.get_field(
+ name, m2m=True, related_objects=True,
+ related_m2m=True, virtual=True

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

add trailing comma

@timgraham

timgraham Aug 7, 2014

Member

add trailing comma

django/db/models/options.py
@cached_property
def concrete_fields(self):
+ """
+ Returns a list of all concrete data fields on the model and it's parents.

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

its (In case it's unclear, "it's" means "it is" which doesn't make sense). Please check all the docstrings so I don't have to comment every place.

@timgraham

timgraham Aug 7, 2014

Member

its (In case it's unclear, "it's" means "it is" which doesn't make sense). Please check all the docstrings so I don't have to comment every place.

django/db/models/options.py
@@ -22,6 +31,32 @@
'index_together', 'apps', 'default_permissions',
'select_on_save', 'default_related_name')
+IMMUTABLE_WARNING = (
+ "get_fields() return type should never be mutated. If you want "

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

The return type of get_field()

@timgraham

timgraham Aug 7, 2014

Member

The return type of get_field()

django/db/models/options.py
@@ -22,6 +31,32 @@
'index_together', 'apps', 'default_permissions',
'select_on_save', 'default_related_name')
+IMMUTABLE_WARNING = (
+ "get_fields() return type should never be mutated. If you want "
+ "to manipulate this list for your own use, make a copy first"

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

add period

@timgraham

timgraham Aug 7, 2014

Member

add period

django/db/models/options.py
+
+
+@lru_cache(maxsize=None)
+def _map_model(opts, link):

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

is link what we call rel in the rest of the code? link has me thinking <a href="...">. Naming is hard.

@timgraham

timgraham Aug 7, 2014

Member

is link what we call rel in the rest of the code? link has me thinking <a href="...">. Naming is hard.

This comment has been minimized.

@PirosB3

PirosB3 Aug 8, 2014

Contributor

link is the new name for (ex) connection

@PirosB3

PirosB3 Aug 8, 2014

Contributor

link is the new name for (ex) connection

django/db/models/options.py
@@ -110,6 +177,59 @@ def app_config(self):
def installed(self):
return self.app_config is not None
+ @property
+ def verbose_name_raw(self):

This comment has been minimized.

@timgraham

timgraham Aug 7, 2014

Member

Everything is still moved around in this file making review difficult.

@timgraham

timgraham Aug 7, 2014

Member

Everything is still moved around in this file making review difficult.

@PirosB3

This comment has been minimized.

Show comment
Hide comment
@PirosB3

PirosB3 Aug 8, 2014

Contributor

@timgraham old formatting of models/options has been restored.

Contributor

PirosB3 commented Aug 8, 2014

@timgraham old formatting of models/options has been restored.

@PirosB3

This comment has been minimized.

Show comment
Hide comment
@PirosB3

PirosB3 Aug 8, 2014

Contributor

@timgraham @freakboy3742

This is the new get_field() API: PirosB3#6. Do you prefer this or the one on the current PR?
I prefer the new one, it's simpler and there is (not yet) any reason for so much filtering options. It also has smaller memory usage.
Suggestions?

Contributor

PirosB3 commented Aug 8, 2014

@timgraham @freakboy3742

This is the new get_field() API: PirosB3#6. Do you prefer this or the one on the current PR?
I prefer the new one, it's simpler and there is (not yet) any reason for so much filtering options. It also has smaller memory usage.
Suggestions?

@timgraham

This comment has been minimized.

Show comment
Hide comment
@timgraham

timgraham Aug 29, 2014

Member

buildbot, test this please.

Member

timgraham commented Aug 29, 2014

buildbot, test this please.

@PirosB3

This comment has been minimized.

Show comment
Hide comment
@PirosB3

PirosB3 Aug 29, 2014

Contributor

Hi Tim,

It's not this PR anymore, the official one is the newest one from me. I
should kill this pr

On Friday, August 29, 2014, Tim Graham notifications@github.com wrote:

buildbot, test this please.


Reply to this email directly or view it on GitHub
#2894 (comment).


PirosB3

https://github.com/PirosB3

Contributor

PirosB3 commented Aug 29, 2014

Hi Tim,

It's not this PR anymore, the official one is the newest one from me. I
should kill this pr

On Friday, August 29, 2014, Tim Graham notifications@github.com wrote:

buildbot, test this please.


Reply to this email directly or view it on GitHub
#2894 (comment).


PirosB3

https://github.com/PirosB3

@timgraham timgraham closed this Aug 29, 2014

@matthiask matthiask referenced this pull request in django-mptt/django-mptt Oct 2, 2014

Closed

Add support for models where primary key is not Integer type #322

@tomchristie

This comment has been minimized.

Show comment
Hide comment
@tomchristie

tomchristie Dec 19, 2014

Member

Linking to newer ticket for folks browsing this... #3114.

Member

tomchristie commented Dec 19, 2014

Linking to newer ticket for folks browsing this... #3114.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment