Opened 16 years ago

Closed 10 years ago

Last modified 6 years ago

#9722 closed New feature (fixed)

Use pyinotify (where available) instead of polling filesystem every second to detect changes

Reported by: Chris Lamb Owned by: Unai Zalakain
Component: Core (Management commands) Version: dev
Severity: Release blocker Keywords:
Cc: unai@… 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

This patch series adds support for pyinotify to replace the "poll-every-one-second" mechanism in django.utils.autoreload on systems where that library is available.

Inotify is an event-driven notifier for monitoring filesytems changes. Instead of dancing around the filesystem comparing modification times every second, we can simply register all the files we are interested in and let the thread block until one of them changes. When this happens, we reload the server as before.

This has the following advantages over the current polling system:

  • Scales far better to larger projects.
  • Response time to code changes is drastically reduced - the server restarts pretty much instantly instead of having to wait ~0.5 seconds for the reload.
  • More robust change detection - pretty much all forms of modification of the interested file are noticed (in particular, moving the file and editing the file without changing the mtime)
  • Saves battery usage - I have to use --noreload on my laptop (with encrypted filesystems) as it noticably reduces my battery life. Django is killing kittens.
  • Cleaner implementation - Polling is just dirty, and requires us to maintain a huge dict full of modification times.

Whilst inotify is Linux-specific, the patch gracefully degrades if a suitable version of pyinotify is not available. It does not add pyinotify as a dependency to Django.

Attachments (3)

0001-Refactor-functionality-to-get-interesting-files-into.patch (1.2 KB ) - added by Chris Lamb 16 years ago.
0002-Add-support-for-pyinotify.patch (3.2 KB ) - added by Chris Lamb 16 years ago.
Update 0002-Add-support-for-pyinotify.patch
0001-add-support-for-pyinotify.patch (3.2 KB ) - added by ch0wn 14 years ago.
Update for django 1.2.1 and pyinotify 0.7

Download all attachments as: .zip

Change History (35)

comment:1 by Chris Lamb, 16 years ago

This doesn't yet for modules that are lazily loaded - need to hook into the request_finished signal to update the files inotify is waiting on, otherwise we will never update the watch list (as we are blocking on it forever). The inotify interface will get in the way a bit here - will probably require calling oll ourselves with an additional anonymous file descriptor which (if written to) will tell us to update our watch list.

comment:2 by Chris Lamb, 16 years ago

Has patch: unset

by Chris Lamb, 16 years ago

Update 0002-Add-support-for-pyinotify.patch

comment:3 by Chris Lamb, 16 years ago

Has patch: set

No need for file descriptor nonsense; we can simply update the inotify watch list from the thread the request_finished callback is called from. Anyway, current patch fixes the lazily loaded modules issue.

comment:4 by Jacob, 16 years ago

Triage Stage: UnreviewedDesign decision needed

by ch0wn, 14 years ago

Update for django 1.2.1 and pyinotify 0.7

comment:5 by ch0wn, 14 years ago

I combined and updated the previous patches and attached one that works with django 1.2.1 and latest pyinotify.

comment:6 by Gabriel Hurley, 14 years ago

Component: django-admin.py runserverCore (Management commands)

comment:7 by Luke Plant, 14 years ago

Severity: Normal
Type: New feature

comment:8 by Aymeric Augustin, 13 years ago

Easy pickings: unset
Triage Stage: Design decision neededAccepted
UI/UX: unset

comment:9 by Unai Zalakain, 11 years ago

Cc: unai@… added
Needs tests: set

comment:10 by Aymeric Augustin, 11 years ago

Needs documentation: set
Patch needs improvement: set

I left a few comments on the pull request. Overall this looks pretty good.

It still needs documentation. I'm not sure how we can write tests.

comment:11 by Unai Zalakain, 11 years ago

I squashed the suggested changes into the old commit, please, let me know your opinion.

I'm sure how to test this either. Were do you suggest to add documentation?

comment:13 by Unai Zalakain, 11 years ago

OK, added a short sentence stating that pyinotify is used for autoreloading when available.

comment:14 by Unai Zalakain, 11 years ago

Owner: changed from nobody to Unai Zalakain
Status: newassigned

comment:15 by Unai Zalakain, 11 years ago

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset
Triage Stage: AcceptedReady for checkin

comment:16 by Tim Graham, 11 years ago

Needs documentation: set

Please don't mark your own patch Ready for Checkin -- another person who reviews the patch should do that. I think this could use a bit more documentation as described in the PR.

comment:17 by Tim Graham, 11 years ago

Triage Stage: Ready for checkinAccepted

comment:18 by Unai Zalakain, 11 years ago

Sorry for the bad ticket handling, won't happen again. PR updated with suggested changes.

comment:19 by Aymeric Augustin, 11 years ago

Needs documentation: unset
Triage Stage: AcceptedReady for checkin

The pull request looks good to me.

However I can't really try it because I'm not using Linux for my development environment.

I would also suggest that the committer summarize and tone down the justification for the feature; it's a bit over the top right now eg. "Django is killing kittens". I get the humor, but not everyone will!

comment:20 by Aymeric Augustin, 11 years ago

(I'm interested because I'd like to implement the equivalent behavior with kqueue on OS X, and this patch lays the groundwork.)

comment:21 by Unai Zalakain, 11 years ago

When I copied the original justification, I wasn't sure what to do with that sentence :p It's now deleted ;)

comment:22 by Claude Paroz, 11 years ago

Apart from minor cleanup with trailing spaces, I can confirm the patch is working well on my Linux machine.

comment:23 by Unai Zalakain, 11 years ago

Version: 1.0master

comment:24 by Tim Graham <timograham@…>, 11 years ago

Resolution: fixed
Status: assignedclosed

In 15f82c701161b327cef54b469c00b6cbe01534db:

Fixed #9722 - used pyinotify as change detection system when available

Used pyinotify (when available) to replace the "pool-every-one-second"
mechanism in django.utils.autoreload.

Thanks Chris Lamb and Pascal Hartig for work on the patch.

comment:25 by Tim Graham, 10 years ago

Has patch: unset
Severity: NormalRelease blocker
Triage Stage: Ready for checkinAccepted

As noted in #22958, this doesn't seem to actually be improving performance. I'll revert it before the 1.7 release if no one wants to try to fix it before then.

comment:26 by Claude Paroz, 10 years ago

Has patch: set

Tentative pull request (tested that performance regression is gone for my use case):
https://github.com/django/django/pull/2883

comment:27 by Unai Zalakain, 10 years ago

Thank you for your time claudep!

comment:28 by Tim Graham, 10 years ago

Resolution: fixed
Status: closednew
Triage Stage: AcceptedReady for checkin

Looks good.

comment:29 by Claude Paroz <claude@…>, 10 years ago

In 6d302f6396b7531eaa22dc5b544d233334e6fc92:

Fixed pyinotify performance regression in 15f82c7011

Refs #9722. Thanks Tim Graham for the review.

comment:30 by Claude Paroz <claude@…>, 10 years ago

In 1bb8ccdb9e70bee35759f2ada4d661481852ab95:

[1.7.x] Fixed pyinotify performance regression in 15f82c7011

Refs #9722. Thanks Tim Graham for the review.
Backport of 6d302f639 from master.

comment:31 by Claude Paroz, 10 years ago

Resolution: fixed
Status: newclosed

comment:32 by Tim Graham <timograham@…>, 6 years ago

In c8720e76:

Fixed #27685 -- Added watchman support to the autoreloader.

Removed support for pyinotify (refs #9722).

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