#22867 closed Cleanup/optimization (fixed)

Pickling models is slow in alpha versions

Reported by: Suor Owned by: nobody
Component: Utilities Version: master
Severity: Normal Keywords:
Cc: loic84 Triage Stage: Ready for checkin
Has patch: yes Needs documentation: no
Needs tests: no Patch needs improvement: no
Easy pickings: no UI/UX: no

Description

When pickling Django models in alpha version get_version() creates a git subprocess, which is very slow. I suggest memoizing get_version().

Change History (12)

comment:1 Changed 13 months ago by Suor

  • Needs documentation unset
  • Needs tests unset
  • Patch needs improvement unset

comment:2 Changed 13 months ago by loic84

  • Cc loic84 added
  • Patch needs improvement set
  • Severity changed from Normal to Release blocker
  • Triage Stage changed from Unreviewed to Accepted

+1 although I'd only wrap get_git_changeset with lru_cache.

Marking as a release blocker for 1.8 since this is a regression from #21430.

comment:3 Changed 13 months ago by anonymous

Moved @lru_cache() to get_git_changeset().

comment:4 Changed 13 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 80f4487d17a0040e9be35e7ee6ac478bafe6504a:

Fixed #22867 -- Memoized django.utils.version.get_git_changeset().

This improves pickling speed in prelease versions of Django; refs #21430.

comment:5 Changed 13 months ago by Aymeric Augustin <aymeric.augustin@…>

In 01399fa0aaa817281c77bbb4b7d82c6c8487c916:

Revert "Fixed #22867 -- Memoized django.utils.version.get_git_changeset()."

This reverts commit 80f4487 temporarily, because that commit prevented
the djangoproject.com server from building the docs, because it still
uses Python 2.6.

comment:6 Changed 13 months ago by aaugustin

  • Resolution fixed deleted
  • Status changed from closed to new

While this commit was correct, I had to revert it because it broke the docs builds, which are still running on Python 2.6.

We need to upgrade the djangoproject.com server before we can push this change. Sorry for the inconvenience.

comment:7 Changed 13 months ago by loic84

Upgrading Python 2.6 which is EOL and recommitting this fix is the ideal course of action.

However if we don't manage to do so within a few of days, I'd suggest we go with a stopgap solution like https://gist.github.com/313430836d20006916c4.

The regression could be quite severe to anyone caching large querysets since we'd be starting a process for each model.

Last edited 13 months ago by loic84 (previous) (diff)

comment:8 Changed 12 months ago by Loic Bistuer <loic.bistuer@…>

  • Resolution set to fixed
  • Status changed from new to closed

In f07735c619c121d8785082972acd8fbad8b236af:

Fixed #22867 -- Memoized django.utils.version.get_git_changeset().

This follows commits 80f4487 and 01399fa; original patch had to be
reverted because it wasn't Python 2.6 compatible and we need it to
be in order to build docs on the djangoproject.com server.

This fix should be replaced by @lru_cache as soon as we drop
Python 2.6 compatibility.

Thanks Florian Apolloner for the review and Alexander Schepanovski
for the original patch.

comment:9 Changed 12 months ago by loic84

  • Easy pickings unset
  • Patch needs improvement unset
  • Severity changed from Release blocker to Normal

Reopening as a "Cleanup/optimization" so we don't lose track of replacing this temporary fix with lru_cache.

comment:10 Changed 12 months ago by loic84

  • Resolution fixed deleted
  • Status changed from closed to new

comment:11 Changed 11 months ago by timgraham

  • Triage Stage changed from Accepted to Ready for checkin

We are just waiting on the new server.

comment:12 Changed 10 months ago by Tim Graham <timograham@…>

  • Resolution set to fixed
  • Status changed from new to closed

In 2c681e8a8c453b1691219eace7ed5dc636e37948:

Fixed #22867 -- Memoized django.utils.version.get_git_changeset().

Restored original fix that had to be removed because the old djangoproject
server was still using Python 2.6.

Note: See TracTickets for help on using tickets.
Back to Top