Created attachment 29808 [details] [review]
patch

Hey, this looks really good to me!

I don't see any problems, system call wise.

* gsize for length, not int.
* check for mmap, not all systems have it.
* win32 has a different function for it.
* MMAP_FAILED not present on all systems.
* Think about the empty-file case.
* g_open, not open, I think.
* mmap is not a win for small files.
* Have a look at gsf_input_mmap_new in libgsf which does exactly what you need.

For systems that don't have a mmap, the caller might want to specify if the file
should be loaded at all. There are many cases where a cahce would be horrible if
the mmap failed and we were loading it with malloc.

About the naming -- I think this would work:

g_file_mapping_new
g_file_mapping_length
g_file_mapping_contents
g_file_mapping_free

POSIX dictates mmap() and MMAP_FAILED.    What systems have one but not the other?

s/MMAP_FAILED/MAP_FILED/

Is there a way to add zero-termination without copying the data ?

From the man page "The system shall always zero-fill any partial page at the 
end of an object."

That should help out...

Unless the file ends on a page boundary...

Good point ;-). Is the zero termination *really* needed?

I cannot think of any useful way to add the zero termination without copying. 
And if we have to copy, there is no point in the mmap(), obviously.

I don't really get the point of the delimiter, though, since the file itself can
contain a NULL, the ->length is always needed.  I say ditch it.

It is not really needed, but it would certainly make the api much more
convenient to use for text files. If we can't make this more convenient than
mmap(), why bother with adding a wrapper ?

re comment 12: because that way people don't need to do the win32 api, and 
mmap, etc and support all the different paths. Generally, it is more useful to 
map binary stuff than it is to do text files, i'd think.

If that's the point, I think we should wrap mmap functionality more generically,
so glib provides a full portable abstraction.

A wrapper for mmap() and munmap() that it easy to use, works for the general use
cases and provides an implementation for all supported platforms, would be a
very much welcomed addition to glib. See for example bug #169792 which would be
a lot easier if glib provided the abstraction and GIMP wouldn't have to deal
with all those #ifdefs.

Retitling for clarity. We need an new API proposal/patch.

Hey, BenM.  Do you know what Win32's file-memory-mapping API looks like?  Is the
consensus that we just want a generic mmap() wrapper and not a more specific
map-readonly thing?

Win32's file memory file-memory-mapping API has more or less the same
functionality as traditional BSD mmap or SysV shared memory. I.e. as long as we
don't use exotic stuff that isn't even portable across Unixes, it should be easy
to implement it also on Windows. For details, google for CreateFileMapping and
MapViewOfFile. One difference that I recall right now is that while on Unix, at
least System V shared memory segments stay around even if the process dies
(which can be a PITA), on Windows file mappings go away if no process has a
handle open to them. (It might be possible to override this behaviour somehow.)

Hey, Tor.  Yah, the SysV shared memory stuff is insane odd.  We'd just wrap
mmap(), which unmaps when the process exits.

+const gchar *
+g_readfile_get_contents (GReadFile *readfile)
+{
+  return readfile->contents;
+}

Shouldn't that be a "const guchar" - pointer? Negative bytes are rather strange,
no? Or just a "void *" (gpointer)?

It should be a "void *" because the returned value isn't necessarily a
null-delimited string; indeed it might be a binary blob.

Otherwise, I'd say go with "char *" since it would be a string.

Traditionally, glib has used "guchar *" for pointers to binary data, because it
is more convenient - you can dereference them, for one.

My API suggestion:

     guchar *g_map_file (const char  *filename,
                         gsize       *length,
                         GError     **err);
     void g_unmap_file (guchar *address);

Notes:

   - Files are always mapped both readable, writable and executable.

   - All files are mapped MAP_PRIVATE

   - You can't map a file partially

   - The size of the file is returned thought gsize *length

   - Implementing this API will require an internal table mapping addresses
     to lengths.

Generally the philosophy is that applications needing the full power of
mmap() should just use mmap() directly.

The internal table doesn't sound like a good idea, adds complexity to the
implementation. Why not have some struct (opaque to the user) that contains any
necessary bookkeeping information?

typedef struct _GFileMapping GFileMapping;

GFileMapping *g_file_mapping_open (const char *filename,
                                   GError    **error);
void          g_file_mapping_close (GFileMapping *mapping);

guchar       *g_file_mapping_get_data (GFileMapping *mapping);
gsize        *g_file_mapping_get_length (GFileMapping *mapping);

Hmm.

It would also be great if the GLib shared memory API would be slightly more
powerful, to cover the relatively simple use case in GIMP. I.e. it should be
possible to also create new shared memory objects that don't correspond to any
actual file, a'la shm_open(O_CREAT)+ftruncate()+mmap(), or the corresponding
System V shm*() calls, or Win32's CreateFileMapping(INVALID_HANDLE_VALUE). 

Maybe:

GFileMapping *g_file_mapping_new (gsize   length,
                                  GError **error);

and something like:

const gchar *g_file_mapping_get_identifier (GFileMapping *mapping);

to get a string (with unspecified opaque contents, typically just an integer)
that can be passed to other processes (or just descendant processes?), which
then can access it with:

GFileMapping *g_file_mapping_open_inherited (const gchar *identifier,
                                             GError     **error);

Perhaps GFileMapping would then not be a good name, as such an object wouldn't
necessarily correspond to a file. 

I agree that we should not expose mmap() sharing semantics, but why not allow
the user to pick a read-only versus r/w mapping?

hmm, it would certainly be nice if we could use the wrapper for the one common
use of mmap in the current code: mmap'ed caches, which are mapped readonly+shared

What about doing private maps?  It's quite like reading the file in a buffer,
but the memory is shared as long as the buffer is not written to.

For null-termination, the manual doesn't make it clear, but doesn't mmap()ing
(filesize+1) bytes work?

Behdad, filesize+1 only works if the size != PAGE_SIZE-1.

Is it really a good idea to mmap text files anyway?  Text files are typically
edited, and it is unspecified whether changes made to the file after the mmap
call are visible in the mapped region.

Matthias, Robert, isn't the difference just that mapping read-only gives you
a segmentation fault on writing, where a read/write mapping would make a copy
of the page? If so, mapping read/write isn't really harmful for the mmap()
caches.

Getting a segfault is a nice touch for debugging, but I don't really think it's 
worth an extra API parameter.

On the other hand a writable (COW) mapping would be useful for mapping
uncompressed pixbufs: the application can legally write to the pixbuf data, but
as long as it doesn't, the memory is shared. If it does write, it just gets a
copy of the page in question which is the exact semantics we are looking for.

Tor, the GFileMapping opaque struct does look better to me.

I am not sure that shared anonymous memory belongs in the same API though. It
seems to me like two unrelated concepts that just happen to be implemented
somewhat similarly.

That may be the case for Linux+glibc. I don't think the SUSv2 specification of
MAP_PRIVATE talks about copy-on-write, but then SUSv2 also does not talk about
sharing the mapping for MAP_SHARED either. It only specifies the effect of 
these flags in terms of who sees changes which are made to the mapped region.

So you are probably right that using MAP_PRIVATE + rwx would work just as well 
for mmap caches.

Behdad: Unless you explicitly care about the sharing semantics, it doesn't
really matter.  And we aren't looking to make a shared memory abstraction; we
want to make an alternative to g_file_get_contents().

Søren: I concur with Matthias.  I think MAP_SHARED makes more sense, but
MAP_PRIVATE will work fine.  But do we want to make it +w?

Robert:  I understand that we aren't looking to make a shared memory
abstraction.  But I'm worried about the case that the applications maps the text
file, and reads it, and then the file is edited, and the map contents may be
changed without the application noticing.  But I think it's not that important
really.  Why should somebody keep a text file around in the memory anyway.

What about this design:


GReadFile *g_readfile_open (const gchar *filename,
                            gsize       maxlen,
                            const char  *mode,
                            GError      **error);

int g_readfile_get_length (GReadFile *readfile);

guchar *g_readfile_get_contents (GReadFile *readfile);

void g_readfile_close (GReadFile *);



The last three are pretty much the same as in the patch, but g_readfile_open
does this:

============================================================================

- stats file and will return null if the file doesn't exists or is not a
  regular file.

- set len = min(filesize, maxlen)

- set nullterminate if character 't' (for text) is found in the mode argument.

- If no mmap available and on win32, try win32 tricks to implement the same
  semantics as of the following mmap case.  Jump to readfallback if mapping fails.

- If no mmap available, jump to readfallback.

- Build mmap protection based on whether the characters 'r', 'w', and 'x'
  appear in mode argument.

- Try mmap MAP_PRIVATE with the built protection and len bytes.  Jump to
  readfallback if mapping fails.  Let buffer be the address returned.

- If nullterminate and len is a multiple of PAGE_SIZE, try to mmap
  MAP_ANONYMOUS a PAGE_SIZE with the same built protection, MAP_FIXED, at
  address buffer+len.  If this fails, munmap the previous map and jump to
  readfallback.

- Jump to out.

readfallback:

- If the character 'm' (for "map") appears in the mode argument, return null.

- Otherwise, malloc len+(nullterminate?1:0) bytes and read len bytes into it.
  Zero the last byte if nullterminate.   Return null if something fails.
  Let buffer be the malloced address otherwise.

out:

- Finally, return the pointer to the GReadFile struct that includes the
  buffer pointer, len, and other housekeeping information.

============================================================================

So what do you think?  The interface is intuitive and extensible IMO.  Text
files can become default and a 'b' char can be used to signal binary.  That
would catch a few bugs probably.

If this is fine, I can implement it in an hour.

Re: Søren's comment #29, you're right, the fact that anonymous shared memory and
memory-mapped-files use partially the same system API is not something that need
to show through in  GLib. Anyway, does anybody else think such functionality
would belong in GLib? I can't immediately think of any other GTK+ application
that would use such functionality than GIMP, so maybe the need for it isn't that
large. On the other hand, I see a risk that people start using the file-mapping
API to map /dev/null and effectively get anonymous shared memory? Should GLib
explicitly restrict the file-mapping API to regular files?

Re: Behdad's comment #33, a minor issue with your suggested API is that no other
GLib API uses an fopen()-style mode string. Flag bitmasks are more or less the norm.

About fopen()-style mode string, right.  That's my personal preference. 
Bitmasks are definitely more efficient and common, but then, they are a huge
pain to use.  I still do 'man 2 open' every single time I want to use open(2).

Anyway, after chatting on IRC, seems like the more practical way is to start
with a MAP_PRIVATE+rw api (why on earth rwx?).  One minor problem with that (as
opposed to read-only maps) is that the Linux+glibc, according to the manual,
reserves swap space for the mapping, such that copy-on-write never fails.  That
may be less than optimal on low-memory systems.

Looks reasonable in general, but I agree with Tor that for GLib, a flags
enumeration would be more appropriate than a string.  

What is the relevance of max_size ?

get_length should probably return gsize, not int

Does close free the structure ?

> (why on earth rwx?).

Basically because of a sick idea I had for something quite different :-). Just
ignore it.

> One minor problem with that (as opposed to read-only maps) is that the
> Linux+glibc, according to the manual, reserves swap space for the mapping,
> such that copy-on-write never fails.  That may be less than optimal on
> low-memory systems.

I guess a single "gboolean writable" parameter might not be that bad. The swap
point is a good one; the system would have to reserve swap space for the mapping
for every appliation mapping the file.

max_size if definitely useless.  Will drop.  I will use flags enumeration.  Yes,
close frees the structure, unmaps, etc.

So I will send a draft patch later.  I need some help with Windows stuff, but
Tor is willing to help I think.

You should be able to see the necessary win32 apis in gkticoncache.c

Created attachment 47878 [details]
Initial code for review

Here is an standalone program with new implementation and test cases.  No WIN32
tricks yet.  Unfortunately I get weird segfaults.  Any idea?  The mystery
should be in the anonymous null-termination page...  Is it just too scary to
bother and we better straightly go allocate memory if nulltermination is asked
for and filesize is a multiple of pagesize?

I think the reason for the segfault is that the anynymous page your code wants
to allocate at the fixed address in the zero-termination case happens to be
already allocated by the C runtime startup, the dynamic loader, or something,
for its own purposes. (Also with MAP_PRIVATE and MAP_ANONYMOUS.) In that case
mmap() happily reuses that mapping. Then your code unmaps that page, which
understandably causes trouble for the C runtime as a page that it uses for
something internally gets ripped out from under its feet, and you get a crash
when main() returns. 

I guess the code could first use mprotect() to check whether the page in
question already is mapped. But that opens up a race condition, another thread
might happen to allocate that same page for some other reason between the
mprotect() and mmap(). Hmm, why doesn't mmap() have a MAP_FRESH (or whatever)
flag that would mean "I wan't a freshly allocated page or nothing at all, dammit"?

Run the program under gdb, set a breakpoint at main. When it stops there, run
pmap on the process. In my case it shows:

5918:   /home/tml/47878
08048000      8K r-x--  /47878
0804a000      4K rw---  /47878
b7e33000      4K rw---    [ anon ]
b7e34000   1160K r-x--  /libc-2.3.2.so
b7f56000     36K rw---  /libc-2.3.2.so
b7f5f000      8K rw---    [ anon ]
b7f61000    492K r-x--  /libglib-2.0.so.0.600.3
b7fdc000      4K rw---  /libglib-2.0.so.0.600.3
b7fe9000      8K rw---    [ anon ]                <== note this
b7feb000     84K r-x--  /ld-2.3.2.so
b8000000      4K rw---  /ld-2.3.2.so
bffeb000     84K rw---    [ stack ]
ffffe000      4K -----    [ anon ]

Then set a breakpoint at the mmap call where the program allocates the
null-termination page. Check what page it is trying to allocate. Verify with
pmap that the address in question is already allocated. Single-step over the
mmap call, and note the return value. Yup, it uses the existing mapping.

We really should not use MAP_FIXED, at all.

I would suggest 

   - Nul termination cannot be done safely. Just don't try at all.

   - Drop the flags, and have a single "gboolean writable"

   - Never try to fread() the file. If mmap() fails, just report the error.
     People who want the fallback can easily get it with g_file_get_contents().

   - I would call the struct something like GMappedFile.

Random thing I noticed:

   - if g_open() fails, the patch returns NULL without setting *error.

Agreed. And don't use MAP_COPY, it isn't Posix, and we don't want the
semantics of our APIs to depend on the presence or absence of nonstandard
extensions.

Tor: Ah, thanks a lot for debugging.

We surely do not need this to fread the file?  The problem with not freading is
that the application should remember which method was successful (mapping or
reading) to use the correct cleanup routine, and that's not optimal IMO.

One more thing:  Now that we are dropping all the extra flags and features:

  - Should we drop the struct and use return the buffer and set *length, like
g_file_get_contents is?  The problem is that to free the map we need that the
user passes the length.

  - Instead of having a writable flag, should we use two different functions,
one returning gchar *, another const gchar *?  That enforces some type checking
that can avoid weird segfaults later.

No, keep the struct.

One more thing that occured to me is that it might be nice to have something
like g_mapped_file_is_stale() which would check if the file has changed since
the mapping was established. That is a common operation for all the "mmap-cache"
use cases.

I think we should also start to think about docs for the new function. The docs 
should mention that modifications of the underlying file may affect the contents
of the GMappedFile, and recomment that GMappedFile should only be used for files
which are

a) never modified during normal operation
or
b) atomically replaced 
or 
c) locked

I would just like to point out APR's mmap abstraction:
http://svn.apache.org/repos/asf/apr/apr/trunk/mmap/
Documentation here: http://apr.apache.org/docs/apr/group__apr__mmap.html

To answer the question in comment #5, Irix 5.3 seems to be an example
of mmap() without MAP_FAILED, see bug 308449

Here is my own proposal for a very direct mmap wrapper. 
Does this look 
 a) agreeable 
 b) correct, documentation-wise 
 c) implementable on Windows ?

Created attachment 48234 [details]
gmappedfile.h

Created attachment 48235 [details]
gmappedfile.c

Re c), as far as I can see (and understand the Win32 API documentation), yes.
But only experimentation will tell whether writable/non-shared will actually work.

The documentation for g_mapped_file_new() uses "they may not be written back".
Would it be clearer to say "they MIGHT not be written back"?

Looks fine to me.

  - You later add a modification time field to the struct to implement
g_mapped_file_is_stale(), right?

  - What about wrapping msync, is it needed?

Picky stuff:

  - In docs, you don't mean "wether", do you? ;)

  - Typo: MA_PRIVATE -> MAP_PRIVATE

Tor, could you do a quick experiment to see if writable/non-shared will work ?
If not, we will be better off without those flags. I'll try to write a little 
test for checking if writable/shared work later today.

Behdad, I'm not so sure about the is_stale() idea anymore. First of all,
the gtk icon cache doesn't actually do that check, and if you think about it,
it is not trivial to omplement. You can not simply unmap the cache which is in
heavy use by GTK+. Instead, you have to properly replace all references to the
old cache by references to the new cache. Now, we do have that in place, for 
icon theme changes, but you have to make sure that changing from a theme to
itself actually works to reopen the cache.

Also the semantics of is_stale() are not totally obvious. Do you keep the
filename around and stat that, or do you keep the file descriptor around and
fstat that ? I believe that will give different results, if the underlying
file has been atomically replaced. Also, the mmap specs seem to indicate
that not all changes in the shared buffer are necessarily reflected in
mtime changes of the underlying file.

So this may need some more thought.

The implementation looks correct to me, but I don't really see the use for the
"shared" flag though. For readable mappings it doesn't make any difference, and
writable/shared mappings are not that much used in practice. Also, two adjacent
booleans in the interface makes it much easier to get them mixed up: "Does
writable come before shared or is it the other way around"?

I agree that if writable/non-shared doesn't work on Windows, then writable
shouldn't be supported at all.

I also agree that the is_stale() method is probably a bad idea.

Tiny thing about the implementation: maybe check the error location with

    g_return_val_if_fail (!err || *err == NULL, NULL)

Created attachment 48280 [details]
a test

Here are some tests

Soeren, your proposal is to

a) do away with the shared parameter, always MAP_PRIVATE
b) verify that writable/non-shared mappings can actually be
   implemented on Windows, if not, drop writable from the api as well ?

Yes, that is my proposal

Created attachment 48283 [details]
simplified version

Created attachment 48284 [details]
simplified header

Created attachment 48285 [details]
simplified test

Created attachment 48301 [details]
win32 file mappping test program for writable/non-shared

A small test program that verifies that writable/non-shared works on Windows,
too. (Using PAGE_WRITECOPY for the protection of the file view in the
CreateFileMapping() call, and then using FILE_MAP_COPY for the access to the
mappded pages in the MapViewOfFil() call.)

And once I corrected the fopen mode to "r+b", and added the possibility to
select writable/shared with a command line option, that too indeed works. Then
one uses PAGE_READWRITE in the CreateFileMapping() and FILE_MAP_WRITE in the
MapViewOfFile().

Thanks for testing Tor. Lets do without sharing for now, even if it
works on win32. If it turns out to be an important feature, we can
always add a g_mapped_file_new_shared().

2005-06-24  Matthias Clasen  <mclasen@redhat.com>

	Add an mmap() wrapper called GMappedFile. (#148218,
	David Schleef, Behdad Esfahbod)
	
	* glib/gmappedfile.[hc]: New files.

	* configure.in: Check for mmap.
	
	* glib/Makefile.am: Add new files.
	
	* glib/glib.symbols: Add new functions.

	* glib/glib.h: Include gmappedfile.h

	* tests/mapping-test.c: Tests for GMappedFile.

	* tests/Makefile.am: Add new file.

Nice to see this commited.

I don't know about the glib standards for this, but I think we can change the
API in this way:

gsize        g_mapped_file_get_length   (const GMappedFile  *file) G_GNUC_CONST;
gchar       *g_mapped_file_get_contents (const GMappedFile  *file) G_GNUC_CONST;

This way it's both clear to the user and to the compiler that these two
functions are cheap and simply return a precomputed value.

Oops, that should read G_GNUC_PURE, not G_GNUC_CONST.

About is_staled, all agreed, but to faciliate stale checking for applications,
and since we already are stat'ing the file, what about having a
g_mapped_file_get_mtime() that returns the modification time of the file at the
time of mapping?


And finally, seems like this:

  g_return_val_if_fail (!error || *error == NULL, NULL)

is typically written this way:

  g_return_val_if_fail ((error == NULL) || (*error == NULL), NULL);

:)