Created attachment 47397 [details] [review]
Patch that will replace the fread stuff with mmap

It has some ifdefs. Thats because I'm not sure whether mmap is supported on all
platforms. Feel free to remove those ifdefs if mmap actually is supported on
all platforms on which Gtk+ currently runs.

See also bug #148218, which is about an mmap() wrapper for glib.

It would be nice to have some timings before/after this patch gets applied. 
Probably both with cold and warm caches.

Wait, creating new diff with some improvements (munmap and check on HAVE_MMAP
rather than on sys/mman.h)

Federico: About bug #148218: gtkiconcache.c is also usind dirty ifdef-tricks to
overcome the mmap-on-win32 issue. So also gtkiconcache.c should be fixed once
this mmap-glib wrapper is in (is it in?).

Created attachment 47399 [details] [review]
Forget the prev one. This one is better

Created attachment 47400 [details] [review]
No no! this one!

I had some conditional/unconditional stuff wrong (the fdin variable).

Loaders have two ways of loading images:

1. if they implement the load() method, then gdk-pixbuf-io.c feeds them a FILE
*.  Loaders which have a load() method should use mmap() themselves.

2. with begin_load() / load_increment().  This is what gdk-pixbuf-io.c uses if
the loader does not implement load().  Then, the loader gets fed chunks of
memory.  In this case, gdk-pixbuf-io.c should use mmap() itself.

Note that it is done this way for historical reasons.  Originally loaders had
both load() for synchronous operation and begin_load()/load_increment() for
incremental operation.  Then, we saw that load() could be made into a wrapper
for incremental loading, and loader modules could just implement *that* instead
of both variants.

I noticed that gdk_pixbuf_new_from_file_at_scale could use some mmap-love. Other
than that (which would fix this concern), I question/wonder: Is using 4069 bytes
on the stack a good idea? the at_scale method is doing that :-\.

Re Comment #8: some image libraries only offer a FILE * based api (libtiff comes
to mind), and thus can only support load(), not the incremental api.

About comment #10 - in that case, those libraries lose :)  If anyone cares about
them, they can optimize them directly.  There's not much we can do for them at
the gdk-pixbuf level.

Maybe I am just stupid, but why do people think that mapping the image files
will buy us anything substantial? They still have to be uncompressed.

If we could get the files stored uncompressed on disk (or compressed in memory),
then we would be getting somewhere.

comment 12:  You don't have to copy the data two times:  From kernel buffer to
stream buffer and from there to user buffer.  Moreover, you share the memory
with other processes.

When you uncompress the files, you are copying into malloc()ed memory that is
not mapped into other applications, so the data ends up not being shared.

I am willing to believe that you save copying the data from memory to memory
once, though. 

We can't really know until we time it.

However, I'd say that the main slowdown is slow image libraries.  Compare
libjpeg's loading speed to Photoshop's, and cry.

The point is, what Robert Love said at GUADEC does not apply here. We are at
best saving a little bit of memcpy() on something that doesn't show up on
profiles ...

Right, but we talked about this extensively with Robert Love yesterday on IRC,
and he said mmap()ing doesn't harm anyway.  But as Paul said, there are other
places in gdk-pixbuf-io.c that can use mmap.  So we can wait for a while to see
more patches.

sure, mmap doesn't hurt, but s/fread/mmap/g is not the answer. The two places
where mmap will make a really big difference (at least potentially) are mmap
caches for fontconfig and gconf

Do you mean we should /not/ mmap smaller files?

fontconfig will be fixed by the end of Summer, thanks to Google bounties :).
I'll have a look at gconf.

Making the code more complex and harder to read for nano performance gains isn't
really worth it, in my opinion.

Then I suggest we finish the api in bug 148218 and export a clean simple API (no
structs involved preferably) for opening files readonly, and use it everywhere
unconditionally.  Is that ok?

Comment #19: GConf uses libXML for it's default backend. So what can be done is
to mmap the XML-file, parse that mmap'd buffer with xmlParseMemory, etcetera. 

Looks trivial:

backends/xml-dir.c:1380

if (!g_file_get_contents (filename,
                            &text,
                            &length,
                            err))
    return NULL;
doc = xmlParseMemory (text, length);

Hey Behad (Comment #21 and #22), I created a small testcase for xml-dir.c at Bug
#307001

No, thats a misunderstanding. The aim for a mmap gconf cache has to be
to 

a) avoid all the parsing. The cache should contain the data in a ready-to-use
  format

b) avoid opening a bazillion small files. There should be a single cache file

Comment #24: So writing the MarkupTree to a file? I don't think it can be done
easily since the memory of that tree isn't continuous (a parent has a doubly
linked list with allocated childs). What might be possible would be to write all
the MarkupEntries to a file, when reloading mmap that file read/write and
reconstruct the MarkupTree from that. I think the MarkupTree will need an 'id'
member. And I think another file could keep an index of those ids (how they form
the tree). Or each MarkupTree will need a char path[1024].

What you think Behdad? Sounds like a fun experiment :-)! I'm interested to try this.

Comment #25 and #24: However ... it will basically mean that we'll be building a
database engine into gconf. So perhaps it's a better solution to just use an
existing fast database implementation (sqlite, etcetera?) ... fontconfig will
probably have the same problem.

You can look at the icon theme cache to get an idea what such a mmap cache could
look like.

fontconfig has pregenerated caches, so I don't see how a database is really a
win there.

For gconf it makes sense, but not something SQL based...

Ignoring the bug completely driving off the GTK+ road and returning to the
original subject: what would need to be done to convince us to apply something
like the original proposal (of using mmap inside gdk-pixbuf) is to do
some measurements indicating that it makes a difference.

Reading the comments and looking at the code myself I don't think it'll make a big difference. Mostly the fact that the images need to be decompressed anyway kinda makes mmapping them pointless. An mmap of the rgb pixmap would be more interesting.

Feel free to mark as wontfix or obsolete.

Agreed.