Created attachment 92068 [details] [review]
MAP_SHARED, ref, unref

also ref/unref and move to g_slice

Looks reasonable to me. Needs to wait for the new devel cycle to open though.
Also needs proper review.

Ryan said he doesn't need this anymore. 
So lets hold off on this until someone else needs shared mappings

Actually, lets leave it open as a dupe-trap

The ref/unref/gslice part is still useful.

Created attachment 136659 [details] [review]
[PATCH] Bug 458796 – refcounting for GMappedFile

(In reply to comment #5)
> The ref/unref/gslice part is still useful.
> 

agreed.  i want to commit this part of it.

+ * Return value: a newly allocated #GMappedFile which must be unref'd
+ *    with g_mapped_file_unref(), or %NULL if the mapping failed. 

If you go so far to replace _free in doc references, should it be deprecated ?


 * Atomically decrements the reference count of @file by one.
+ * If the reference count drops to 0, unmaps the buffer of @file and
+ * frees it. This function is MT-safe and may be called from any thread.


I kinda dislike this style of documentation (even if it is just copied from ghash.c). "MT-safe" is not really a good term to use in the docs, I think.
And it always makes me quiver a little bit if ref/unref docs are written in terms of incrementing/decrementing a count, and the fact that that happens atomically is really just an implementation detail.


Do we have any users for this ?

i want to use this.  it's why i was inspired :)

my argument against deprecation is that we didn't do it in the case of g_hash_table_destroy().  not a strong argument.  i'm happy either way.

i'll change the docs.

g_hash_table_destroy doesn't do the same thing as g_hash_table_unref...

sometimes it does the same thing -- ie: for the last reference.

for the other cases you could use g_hash_table_remove_all() -- _destroy() could easily be deprecated.

in any case, i am sensing that you prefer deprecation here -- will post a new patch.  :)

Created attachment 136676 [details] [review]
[PATCH] GMappedFile: add refcounting, switch to GSlice

sorry for the delay :)

Looks ok, but you took my complaint about the docs too literally; it is still worth pointing out that ref/unref are safe to be called from any thread without locking, unlike other mapped file functions.

If you add some blurb about thread-safety (without technobabble like "MT-safe"), its fine to commit. But I'd ask you to also clear GTK+ of the newly deprecated api after you did so, and bump the GLib dep in GTK+ configure.in.

Committed with changes.  Leaving the bug report open for the same reason it was open before.

*** Bug 631061 has been marked as a duplicate of this bug. ***

The patch allows to use MAP_SHARED. It means that dirty pages will be eventually flushed back to file.

But the moment of flushing is not defined. As far as I understand it might happen right after change or at munmap. 

But sometimes you need to force flushing. What do you think about adding something like g_mapped_file_sync(GmappedFile *) that invokes msync or WinNT equivalent?

Or there is a better way to manage cross-platform memory mapped files?

Files must be mapped MAP_SHARED or MAP_PRIVATE.

Your statements lead me to believe that you'd rather not use MAP_SHARED, which means you must want to use MAP_PRIVATE.

Calling msync() on MAP_PRIVATE does absolutely nothing, for the reason I mentioned in bug 631061.  See http://www.opengroup.org/onlinepubs/009695399/functions/msync.html:

"""
When the msync() function is called on MAP_PRIVATE mappings, any modified data shall not be written to the underlying object and shall not cause such data to be made visible to other processes.
"""

One thing that can be known, though, is that with any decent OS, using MAP_SHARED results in you sharing the copy of the page with that is in the page cache.  This means that all processes mapping the file are using the same physical memory and any changes made will be seen right away and (assuming you make appropriate arrangements) the writes will even be visible in exactly the order they occur.

(In reply to comment #16)

> One thing that can be known, though, is that with any decent OS, using
> MAP_SHARED results in you sharing the copy of the page with that is in the page
> cache.  This means that all processes mapping the file are using the same
> physical memory and any changes made will be seen right away and (assuming you
> make appropriate arrangements) the writes will even be visible in exactly the
> order they occur.

How about multi-core?  I think that's a huge lot of assumptions.

One difference between MAP_SHARED and MAP_PRIVATE, at least for writable mappings, is that a writable PRIVATE mapping allocates swap space to ensure that modifying the area never segfaults.  Whereas MAP_SHARED or MAP_PRIVATE|MAP_NORESERVE doesn't reserve swap.  So, for things that are not expected to be modified (icon cache, fonts, etc), writable MAP_PRIVATE is suboptimal.  Normally the answer is: don't make it writable then, however, since g_mapped_file doesn't have API to change from readonly to writable after creation, there are legitimate usecases where modification is the exception, but one has to create a writable mapping just in case.

My comment about 'make appropriate arragements' was meant to address the issue of cross-processor cache consistency (use of barriers or sync primatives or what-have-you).

Also note that MAP_PRIVATE, in practice, tends -not- to allocate sufficient swap space due to the practice of memory over-commit by the kernel.

*** Bug 720184 has been marked as a duplicate of this bug. ***

Bug #720184 had some patches for this too; they have the advantage over these patches that they support creating a shared mapping from an existing open FD.

-- GitLab Migration Automatic Message --

This bug has been migrated to GNOME's GitLab instance and has been closed from further activity.

You can subscribe and participate further through the new bug through this link to our GitLab instance: https://gitlab.gnome.org/GNOME/glib/issues/99.