Django

Code

Ticket #3436 (closed: wontfix)

Opened 2 years ago

Last modified 1 year ago

Don't fetch Field's choices on form/manipulator creation

Reported by: Ivan Sagalaev <Maniac@SoftwareManiacs.Org> Assigned to: nobody
Milestone: Component: Core framework
Version: SVN Keywords: performance
Cc: Maniac@SoftwareManiacs.Org, gabor@nekomancer.net Triage Stage: Accepted
Has patch: 1 Needs documentation: 0
Needs tests: 0 Patch needs improvement: 1

Description

Currently creation of a default manipulator for a model causes actual fetching of all content of every related parent models which tends to be slow. They are used to fill <select>s created for ForeignKeys? and ManyToManyFields? but this may be unnecessary if those <select>s never rendered. This can be easily fixed by turning a method that gets choices for <select>s (Field.get_choices()) into a generator that will hit database only when (and if) needed.

Patch follows.

Attachments

3436.diff (1.4 kB) - added by Ivan Sagalaev <Maniac@SoftwareManiacs.Org> on 02/05/07 15:26:54.
Patch

Change History

02/05/07 15:26:54 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

  • attachment 3436.diff added.

Patch

02/05/07 15:27:34 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

  • needs_better_patch changed.
  • has_patch set to 1.
  • needs_tests changed.
  • needs_docs changed.

03/05/07 09:41:52 changed by Gary Wilson <gary.wilson@gmail.com>

  • needs_better_patch set to 1.
  • stage changed from Unreviewed to Accepted.

Marking this as accepted, but new feature will probably not get added to Manipulators. Does newforms exhibit the same problem? If so, it would be great if someone could submit a patch for newforms.

03/05/07 09:56:35 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

Gary, this code lives in db field, not old form fields, so it works for newforms too. Actually it was created after concerns in django-developers about newforms fetching related values like old forms.

03/12/07 07:50:09 changed by mtredinnick

If we ever need it for verifying this problem, Joe Heck put a test case in #2638.

06/01/07 01:14:30 changed by mtredinnick

  • component changed from Database wrapper to Core framework.

06/02/07 16:08:59 changed by anonymous

  • cc changed from Maniac@SoftwareManiacs.Org to Maniac@SoftwareManiacs.Org, gabor@nekomancer.net.

06/13/07 07:22:22 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

  • keywords set to performance.

06/13/07 08:01:11 changed by mtredinnick

A small comment about this change (since I've looked at it, but don't have cycles to complete the work at the moment)...

I looked at this patch a couple of weeks ago. It wasn't immediately clear where the performance improvement appeared. It's definitely an improvement for some uses, but are we hitting those uses in newforms?

So we need a test to demonstrate how this is triggered, I think. It's a slight backwards incompatible change (since the returned type of get_choices changes). It wasn't apparent when I moved Joe Heck's example from #2638 over to newforms, for example.

06/13/07 08:07:11 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

To clarify... You want a regression test showing that it works properly or a test showing a performance gain? I'm not sure how to do the latter. And the former is show by any newforms test that renders a <select>.

07/04/07 06:06:17 changed by Ivan Sagalaev <Maniac@SoftwareManiacs.Org>

Ok, I've tested it a bit. The problem really doesn't affect newforms since Field.formfield calls get_choices only if a field has explicit 'choices' attribute set. ForeignKeys? don't have choices and are fetched by formfield's widget only at display time. Oldforms still have a problem though.

09/16/07 07:59:29 changed by ubernostrum

  • status changed from new to closed.
  • resolution set to wontfix.

Per comments above this is not a problem in newforms, and the old forms system is deprecated, so I'm marking this wontfix.

Add/Change #3436 (Don't fetch Field's choices on form/manipulator creation)




Change Properties
Action