Opened 8 years ago

Closed 2 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: master
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 8 years ago.
0002-Add-support-for-pyinotify.patch (3.2 KB) - added by Chris Lamb 8 years ago.
Update 0002-Add-support-for-pyinotify.patch
0001-add-support-for-pyinotify.patch (3.2 KB) - added by ch0wn 6 years ago.
Update for django 1.2.1 and pyinotify 0.7

Download all attachments as: .zip

Change History (34)

comment:1 Changed 8 years ago by Chris Lamb

Needs documentation: unset
Needs tests: unset
Patch needs improvement: unset

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 Changed 8 years ago by Chris Lamb

Has patch: unset

Changed 8 years ago by Chris Lamb

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

comment:3 Changed 8 years ago by Chris Lamb

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 Changed 8 years ago by Jacob

Triage Stage: UnreviewedDesign decision needed

Changed 6 years ago by ch0wn

Update for django 1.2.1 and pyinotify 0.7

comment:5 Changed 6 years ago by ch0wn

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

comment:6 Changed 5 years ago by Gabriel Hurley

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

comment:7 Changed 5 years ago by Luke Plant

Severity: Normal
Type: New feature

comment:8 Changed 5 years ago by Aymeric Augustin

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

comment:9 Changed 3 years ago by Unai Zalakain

Cc: unai@… added
Needs tests: set

comment:10 Changed 3 years ago by Aymeric Augustin

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 Changed 3 years ago by Unai Zalakain

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 Changed 3 years ago by Unai Zalakain

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

comment:14 Changed 3 years ago by Unai Zalakain

Owner: changed from nobody to Unai Zalakain
Status: newassigned

comment:15 Changed 3 years ago by Unai Zalakain

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

comment:16 Changed 3 years ago by Tim Graham

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 Changed 3 years ago by Tim Graham

Triage Stage: Ready for checkinAccepted

comment:18 Changed 3 years ago by Unai Zalakain

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

comment:19 Changed 3 years ago by Aymeric Augustin

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 Changed 3 years ago by Aymeric Augustin

(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 Changed 3 years ago by Unai Zalakain

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

comment:22 Changed 3 years ago by Claude Paroz

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

comment:23 Changed 3 years ago by Unai Zalakain

Version: 1.0master

comment:24 Changed 3 years ago by Tim Graham <timograham@…>

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 Changed 2 years ago by Tim Graham

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 Changed 2 years ago by Claude Paroz

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 Changed 2 years ago by Unai Zalakain

Thank you for your time claudep!

comment:28 Changed 2 years ago by Tim Graham

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

Looks good.

comment:29 Changed 2 years ago by Claude Paroz <claude@…>

In 6d302f6396b7531eaa22dc5b544d233334e6fc92:

Fixed pyinotify performance regression in 15f82c7011

Refs #9722. Thanks Tim Graham for the review.

comment:30 Changed 2 years ago by Claude Paroz <claude@…>

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 Changed 2 years ago by Claude Paroz

Resolution: fixed
Status: newclosed
Note: See TracTickets for help on using tickets.
Back to Top