Re: patch nag: Multithreaded thumbnail loading

[Christian Neumair <cneumair gnome org>]

Am Montag, den 03.09.2007, 10:57 +0200 schrieb Alexander Larsson:
> On Sun, 2007-09-02 at 11:59 +0200, Christian Neumair wrote:
> > I've attached a patch to bug 104224 [1] that implements multithreaded
> > thumbnail loading. I'd appreciate some comments.
> > 
> > Do not confuse this with the thumbnail cache issue discusses in recent
> > blog entries, we'll implement another solution to that.
> > 
> > [1] http://bugzilla.gnome.org/show_bug.cgi?id=104224
> 
> It seems a bit late in the game for this release, but seems like a good
> idea.
> 
> Some quick comments from just glancing over the patch:

Thanks for your incredibly fast reponse!

> I don't think the immediate_thumb_loading argument is needed. We should
> just always read it async.

I thought it might be useful for bookmark lists, the spatial window's
button, the file properties window etc, but you may well be right here.

> Why are you implementing thumbnail reading as a thread? Why not use the
> existing async i/o machinery in gnome-vfs. 

We can entirely reuse the existing loading code, and that IMO makes it
way more traceable, although multithreading is a can of worms.

We just have to protect the cache against parallel access. Of course
your scepticism towards multithreaded programming is well-reasoned, but
it just seemed to be more straightforward since it allowed to completely
reuse the synchronous and well-tested core loader code
(nautilus_thumbnail_load_image). We're really doing lots of I/O, I'm not
sure how well the async machine deals with a few hundred tiny requests
per second.

> thumbnail_loading_thread_func() uses a lot of nautilus stuff in a
> thread, which is not possible. Nautilus just is not threadsafe in this
> fashion. Threads can only do really low-level stuff. 

"a lot of nautilus stuff" is not very precise.

We use nautilus_icon_factory_get_pixbuf_for_icon which in fact doesn't
do anything VFS- or NautilusFile-related, but just does the cache
handling and filling (i.e. does actual image loading). I forgot to lock
the mutex around the g_hash_table_foreach_remove call in
nautilus_icon_factory_sweep, though. We may also want to protect the
recently used list as it is updated through mark_recently_used in
get_icon_from_cache.

We also use NautilusFile, for passing around URIs, and - in the timeout
- for emitting the file_changed() signal.

At least the possibly-harmful passing-around could be removed.
thumbnail_loading_thread_func() will just pass a URI to
thumbnail_loading_notify_file_changed anyway, so passing the thread an
URI as well may be fine.

Regarding the file_changed() NautilusFile usage in
thumbnail_loading_notify_file_changed: We also use similar code for
NautilusThumbnail, in thumbnail_thread_notify_file_changed(), so I
thought it would be safe.

> Does is_loading_thumbnail really need to be its own thing? Can't we just
> re-use is_thumbnailing (that is after all what we're doing from a
> highlevel perspective).

I'm not sure. One day we'll hopefully implement proper thumbnailing
cancellation (through the stop button in the toolbar), I think it may
come in handy to distinguish it from the NautilusIconFactory-related
code, where cancellation isn't neccessary - or, if we decide to also
allow thumbnail loading cancellation, it will have an entirely different
code path.

> Maybe break out the is_loading pixbuf generation code to a helper
> function.

Pardon?

> Maybe the nautilus_thumbnail_prioritize() stuff can be used or extended
> to prioritize loading of thumbnails for currently visible icons.

I'm not sure. Actually, the NautilusThumbnail code has a lot less in
common with thumbnail loading than the NautilusIconFactory code, and
that's why I put it there. Why would we want to prioritize thumbnail
loading, it's supposed to happen instantaneous (well almost), or at
least within 2-3 seconds for virtually arbitrarily large folders. Are
you talking about the situation where one runs out of main memory, and
just wants to show thumbnails for visible files?

-- 
Christian Neumair <cneumair gnome org>