Opened 12 years ago

Closed 11 years ago

#1316 closed defect (fixed)

[PATCH] Use inotify instead of FAM/Gamin on Linux

Reported by: Philip Taylor Owned by: noKid
Priority: Must Have Milestone: Alpha 12
Component: Core engine Keywords: patch simple
Cc: Yves Patch:

Description

Currently we use the FAM API (usually implemented by Gamin, not the original FAM) to detect file and directory changes, so we can reload resources at run-time (useful for developers and modders).

Gamin doesn't seem to be actively maintained (last release was 3.5 years ago) and is a pretty heavyweight library for our needs. It'd probably be cleaner and more future-compatible to use the Linux kernel's inotify API directly, instead of using Gamin as an unnecessary wrapper around it. We don't need to support ancient pre-inotify Linux kernel versions, or non-Linux OSes.

I haven't looked into the details of inotify to see if it actually would work easily, or if there's any substantial trickiness required. Gamin has the advantage of generally working today, so it's probably not worth a huge amount of effort to change to inotify.

dir_watch_fam.cpp is the current implementation of the dir_watch API.

Attachments (6)

dir_watch_fam.cpp.patch (6.6 KB ) - added by noKid 12 years ago.
Gamin / FAM are not needed anymore. Most of the work was to rewrite event processing.
config2.h.patch (379 bytes ) - added by noKid 12 years ago.
Removed FAM preprocessor directives
premake4.lua.patch (1.5 KB ) - added by noKid 12 years ago.
Removed FAM requirements
inotify.patch (16.8 KB ) - added by historic_bruno 12 years ago.
updated combined patch with some cleanup
dir_watch_fam.cpp.2.patch (7.1 KB ) - added by noKid 12 years ago.
2nd version of the dir_watch_fam.cpp patch
inotify2.patch (10.6 KB ) - added by historic_bruno 11 years ago.

Download all attachments as: .zip

Change History (20)

by noKid, 12 years ago

Attachment: dir_watch_fam.cpp.patch added

Gamin / FAM are not needed anymore. Most of the work was to rewrite event processing.

comment:1 by historic_bruno, 12 years ago

Keywords: review patch added
Milestone: BacklogAlpha 10
Summary: Use inotify instead of FAM/Gamin on Linux[PATCH] Use inotify instead of FAM/Gamin on Linux

comment:2 by Kieran P, 12 years ago

Keywords: simple review patch → patch review simple
Owner: set to noKid
Priority: Should HaveNice to Have

Hey noKid. Thanks for your patch. Any chance you could test this on Mac OS X? Hotloading doesn't work on Mac, and it would be nice, if we're changing the system, to be compatible over all three OS's (Win/Linux/Mac).

Edit: Sorry, ignore my comment. I didn't notice your changes were for the linux folder only. Windows already works, and OSX will be taken care of in #514

I noticed too, it doesn't remove FAM/Gamin from any build file requirements? Are there any still linked after this patch change?

Last edited 12 years ago by Kieran P (previous) (diff)

comment:3 by alan, 12 years ago

I find two tiny issues in the patch:

1.FAMCancelMonitor isn't replaced with inotify_rm_watch

2.inotifyfd is better defined as static

3.g_paths.push_back(*dirWatch); will trigger constructor/destructor of DirWatch, and may be not expected? My recommendation is to use std::map<int, PDirWatch> instead.

And after test on ubuntu 11.10,I find there seems to be a bug with inotify that each modification will invoke IN_MODIFY event twice.

Last edited 12 years ago by alan (previous) (diff)

comment:4 by historic_bruno, 12 years ago

To add to previous comments

  • You can remove CONFIG2_FAM from lib/config2.h, similarly you no longer have to include that file (assuming the new implementation is fairly reliable and we don't need to disable it)
  • without-fam option and the related link commands can be removed from premake4.lua
  • Should rename dir_watch_fam.cpp to dir_watch_inotify.cpp or simply dir_watch.cpp
  • Should omit the "removed stub" comment

comment:5 by Kieran P, 12 years ago

Milestone: Alpha 10Alpha 11

comment:6 by historic_bruno, 12 years ago

Priority: Nice to HaveMust Have

comment:7 by historic_bruno, 12 years ago

Elevating this because all the recent reports of the game freezing (usually in libfam) are troubling.

comment:8 by noKid, 12 years ago

Hello,

I apologize for the lack of news. I read with attention your posts, and I provided a second version of the patch, with slight modifications (the ones listed by Alan).

I hope the Ubuntu problem is being resolved (the IN_MODIFY flag has been replaced by IN_CLOSE_WRITE).

About the game freezing, I haven't had any problems. I can't really do something without further infos.

You'll find in the attachements the patch for premake4.lua and lib/config2.h. I'm not yet familiar with the 0ad system, so I don't know if there are other files that need to be changed, and I'm sorry about that.

Also, I saw a recent ticket ( http://trac.wildfiregames.com/ticket/1420 ) about a problem that might occur with relative directories. I'm not 100 % sure about how it would reacts with inotify, but the modification is tiny, and not harmful (IMHO), so I put it in the 2nd patch.

Last edited 12 years ago by noKid (previous) (diff)

by noKid, 12 years ago

Attachment: config2.h.patch added

Removed FAM preprocessor directives

by noKid, 12 years ago

Attachment: premake4.lua.patch added

Removed FAM requirements

by historic_bruno, 12 years ago

Attachment: inotify.patch added

updated combined patch with some cleanup

comment:9 by historic_bruno, 12 years ago

Keywords: review removed

Got around to reviewing the patch(es) :)

I resolved some conflicts with latest SVN, fixed a warning about signed/unsigned comparison, cleaned up most references to FAM, and attached a new combined patch.

There's an issue in how the inotify events are being handled. They actually have a watch bitmask unlike FAM which had a single code per FAMEvent. We should change the field name in NotificationEvent to mask to reflect that, but more importantly treat it like a bitmask (not assuming they are mutually exclusive) since the case tests will fail when multiple bits flags are set. Maybe that means we need to push multiple notifications per event in some cases, or give one type priority over another?

Since you changed IN_MODIFY to IN_CLOSE_WRITE for inotify watches, I think it should be changed in dir_watch_Poll() too.

Related: for some reason, hotloading is not working well in my Ubuntu 11.10 VM, even before your patch. I'm trying to confirm that GUI XML changes are picked up, but nothing happens. It works in Windows, maybe I can try booting into a real Ubuntu system and see if that makes a difference.

comment:10 by Kieran P, 12 years ago

Milestone: Alpha 11Alpha 12

comment:11 by Yves, 12 years ago

Cc: Yves added

comment:12 by Yves, 12 years ago

There's something wrong or at least not optimal in inotify_deinit().

close(inotifyfd); causes select() to fail in inotify_event_loop because it closes the file descriptor that select is watching for changes. This will terminate the thread. Depending on how quickly the thread is terminated, sometimes pthread_cancel returns errorcode 3 (ESRCH No thread with the ID thread could be found). I'm not sure yet how to do it better, maybe I'll have a closer look later.

by noKid, 12 years ago

Attachment: dir_watch_fam.cpp.2.patch added

2nd version of the dir_watch_fam.cpp patch

comment:13 by historic_bruno, 11 years ago

I've figured out why I couldn't get hotloading to work on Ubuntu (see #1687). I tested your patch and it seems to work at least as well as the existing FAM implementation. I'm attaching a new combined patch for convenience.

by historic_bruno, 11 years ago

Attachment: inotify2.patch added

comment:14 by ben, 11 years ago

Resolution: fixed
Status: newclosed

In 12726:

Replaces FAM file monitoring with inotify on Linux, based on patch by noKid. Fixes #1316, refs #1687.
Removes FAM dependency.

Note: See TracTickets for help on using tickets.