#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)
Change History (35)
by , 16 years ago
Attachment: | 0001-Refactor-functionality-to-get-interesting-files-into.patch added |
---|
comment:1 by , 16 years ago
comment:2 by , 16 years ago
Has patch: | unset |
---|
by , 16 years ago
Attachment: | 0002-Add-support-for-pyinotify.patch added |
---|
Update 0002-Add-support-for-pyinotify.patch
comment:3 by , 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 , 16 years ago
Triage Stage: | Unreviewed → Design decision needed |
---|
by , 14 years ago
Attachment: | 0001-add-support-for-pyinotify.patch added |
---|
Update for django 1.2.1 and pyinotify 0.7
comment:5 by , 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 , 14 years ago
Component: | django-admin.py runserver → Core (Management commands) |
---|
comment:7 by , 14 years ago
Severity: | → Normal |
---|---|
Type: | → New feature |
comment:8 by , 13 years ago
Easy pickings: | unset |
---|---|
Triage Stage: | Design decision needed → Accepted |
UI/UX: | unset |
comment:9 by , 11 years ago
Cc: | added |
---|---|
Needs tests: | set |
Updated pull request: https://github.com/django/django/pull/1731
comment:10 by , 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 , 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:12 by , 11 years ago
I would suggest docs in the runserver docs? https://docs.djangoproject.com/en/dev/ref/django-admin/#runserver-port-or-address-port
comment:13 by , 11 years ago
OK, added a short sentence stating that pyinotify
is used for autoreloading when available.
comment:14 by , 11 years ago
Owner: | changed from | to
---|---|
Status: | new → assigned |
comment:15 by , 11 years ago
Needs documentation: | unset |
---|---|
Needs tests: | unset |
Patch needs improvement: | unset |
Triage Stage: | Accepted → Ready for checkin |
comment:16 by , 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 , 11 years ago
Triage Stage: | Ready for checkin → Accepted |
---|
comment:18 by , 11 years ago
Sorry for the bad ticket handling, won't happen again. PR updated with suggested changes.
comment:19 by , 11 years ago
Needs documentation: | unset |
---|---|
Triage Stage: | Accepted → Ready 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 , 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 , 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 , 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 , 11 years ago
Version: | 1.0 → master |
---|
comment:24 by , 11 years ago
Resolution: | → fixed |
---|---|
Status: | assigned → closed |
comment:25 by , 10 years ago
Has patch: | unset |
---|---|
Severity: | Normal → Release blocker |
Triage Stage: | Ready for checkin → Accepted |
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 , 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:28 by , 10 years ago
Resolution: | fixed |
---|---|
Status: | closed → new |
Triage Stage: | Accepted → Ready for checkin |
Looks good.
comment:31 by , 10 years ago
Resolution: | → fixed |
---|---|
Status: | new → closed |
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.