GNOME Bugzilla – Bug 667292
filesrc: madvise() is not available on all platforms
Last modified: 2012-05-15 18:09:58 UTC
Created attachment 204610 [details] [review] patch Not all platforms have madvise(). In fact, more platforms probably have posix_madvise(). Use one of the two based on autoconf check, currently preffering posix_madvise().
Hrm, this is correct of course, but I wonder if we shouldn't just remove the madvise calls and be done with it instead. Not only is the mmap() code path disabled by default, but then the sequential property is FALSE by default as well, and it's all really pretty much unsupported and a bad idea and just a microoptimisation really, and removed in 0.11 already. I also wonder if we really need all that configure stuff, or if we can't just cheat and get away with doing something like: #if defined (POSIX_MADV_SEQUENTIAL) posix_madvise (...) #else defined (MADV_SEQUENTIAL) madvise (...) #endif
Håvard, ping, could you please respond to comment#1 ?
In all honesty, I simply observed that compilation failed because I didn't have madvise() and went about finding a alternative. I don't think we even use file source for anything but testing. So in short, I have no preference one way or the other when it comes to removing the madvise() calls. If you want to keep them though, I'd prefer if configure could do at least some basic checks. If the current ones are overkill, maybe a simpler AC_CHECK_FUNC would suffice?
I would prefer to just remove them then, I doubt they make much sense anyway: commit 7a70c91ac37d1ece87f31d98f9d7d0333f31bfd6 Author: Idar Tollefsen <itollefs@cisco.com> Date: Tue May 15 19:04:39 2012 +0100 configure: explicitly check for sys/mman.h header And use header-specific guards. https://bugzilla.gnome.org/show_bug.cgi?id=667292 commit f85885474cee78065d9aafb8eac7d74aec54cc69 Author: Tim-Philipp Müller <tim.muller@collabora.co.uk> Date: Tue May 15 18:35:58 2012 +0100 filesrc: remove unused or questionable madvise() calls in deprecated mmap code paths use-mmap functionality has been removed in 0.11 and doesn't really have any performance advantages in most cases anyway. Deprecate "sequential" property to hint sequential access in mmap mode and make it non-functional. Chances are no one was using this ever anyway, or inappropriately. Remove madvise() calls. We would need to do extensive configure checks to handle these properly on different systems, and it doesn't seem worth adding that for code that's already removed in 0.11 or questionable anyway (like madvise DONTNEED right before munmap). https://bugzilla.gnome.org/show_bug.cgi?id=667292