N.B.: Most people don't use builds with the code that this bug is complaining about -- it's not enabled in release builds since --disable-gnomeui defaults to doing a configure test for the version, and the build machines don't *currently* have that version, although that could change in the future. But it does show up in builds that people do on relatively new distributions. The GNOME icon code in modules/libpr0n/decoders/icon/gtk/ is really really slow. I've described this on IRC in the past, and some people had useful suggestions. On my machine, it takes a few hundred milliseconds *per icon*. This is a significant slowdown just when opening a dialog that has OK and Cancel buttons (since we do two such icon loads in that case, at least the first time). But it's a much much more serious problem for bookmarks, where we do an icon load for every single file: URL in any displayed bookmarks -- if a bookmarks folder, or the personal toolbar, has a lot of file: URLs in it, this is a huge slowdown, and can lead to multi-second delays waiting for bookmarks menus to open. Using some printf(!) timing, since profiling of things involving X server, etc., can be a little unreliable, the slowest pieces of nsIconChannel::InitWithGnome seem to be: * the call to gnome_icon_lookup, typically around 300ms * the g_object_unref of the icon theme (odd), typically around 50ms * the pixmap manipulation from there to the end of the function varies a lot -- sometimes it's almost instantaneous, sometimes it's near 50ms, sometimes in between When I brought this up on IRC, a bunch of people had suggestions for improving the last part, but that's actually not the bulk of the time. I don't think Mozilla should ship this code in official builds until these problems are resolved.

(And, for what it's worth, the gdk_pixbuf_save call in the third bullet point causes a pile of UMRs reported by valgrind.)

This might help a drop, although really avoiding doing PNG encoding (expensive) would probably help a good bit more. "bmp" and "ico" are apparently alternatives.

Comment on attachment 211426 [details] [diff] [review] avoid going through temp files when gdk_pixbuf_save_to_buffer is available (the 2.4 dependency is why I didn't write it this way) this means that if the build system has gtk 2.4 installed but the user doesn't, it won't work there... I recently thought about this, and the way to fix it is imo to make nsIconDecoder able to accept ARGB data as produced by gdk-pixbuf.

One point, though: the thing you're discussing fixing is only around 20% of the time.

your patch is (partially) addressing the same issue though, right?

You could also just turn the data from gdk_pixbuf into the format nsIconDecoder wants; that's pretty trivial if you have to deal with only a small number of gdk_pixbuf format variants and probably much faster than round-tripping through PNG.

This adds one byte to the Icon decoder's format: a 1 or an 8 to indicate the number of bytes of alpha. (I was considering using a gfx_format, except that's a long, for some silly reason. I guess I could anyway.) This contains the following changes: * Adds a USE_ICON_DECODER makefile variable and define so that it's easier to change, and adds the MOZ_ENABLE_GNOMEUI case to it * documents the change to the Icon decoder data, as well as documenting the existing rules better * implements that change in the Icon decoder, and also cleans up nsIconDecoder::WriteFrom a bit: removes unused variables, prefers PRUint8 over char so that images with dimensions 128 <= dim < 256 work on platforms where char is signed, and does one notify at the end rather than one per line, and adds some rudimentary error handling to avoid reading past the end of a malformed buffer * converts the beos, mac, and os2 icon code to add the (char)1 to their buffer * converts moz_gdk_pixbuf_to_channel to create icon data instead of PNG data by reading data as described at http://developer.gimp.org/api/2.0/gdk-pixbuf/gdk-pixbuf-gdk-pixbuf.html , with error handling to ensure that it's actually what it expects

So it turns out caching the GnomeIconTheme object makes a phenomenal performance difference here. Patch coming shortly...

Note that attachment 211527 [details] [diff] [review] applies on top of attachment 211514 [details] [diff] [review] (and was generated by diff rather than cvs diff).

Wall clock time to open a personal toolbar submenu (i.e., a sub-sub-folder of the personal toolbar folder) with 21 file URLs in it, after a clean start of Firefox and opening the personal toolbar menu containing the submenu, which has one file URL in it. Scrolling to and opening the submenu was done with keyboard instead of mouse for better timing and hopefully avoiding the hover->open delay time. no patch: 8.34 7.59 7.42 with only first patch: 7.65 7.80 7.68 with both patches: 0.83 0.71 0.78 So I actually couldn't detect a statistically significant effect from the first patch with my stopwatch, although it may have reduced the variability.

with only the second patch: 0.87 0.78 0.72 I think I still want to get the first patch in; there are advantages to being more like other platforms, not using files in /tmp, and not going through a PNG encoder that gives UMRs.

(And there could have been a real performance difference that I just couldn't measure with a stopwatch. The margin of error for my 0.* stopwatch timings practically includes 0.)

Comment on attachment 211514 [details] [diff] [review] use the Icon decoder (should fix PNG and file I/O perf issues) nsIconDecoder.cpp + PRInt32 w = *(data++); + PRInt32 h = *(data++); + PRInt32 alphaBits = *(data++); hm, why signed? + mFrame->SetImageData(data, bpr, rownum*bpr); + mFrame->SetAlphaData(data, abpr, rownum*abpr); I'd put spaces around the *... beos/nsIconChannel.cpp + buffer[2] = 1; // alpha bits per pixel // RGB data uint8* destByte = buffer + 2; doesn't this need to be +3 now? gtk/nsIconChannel.cpp + guchar * const pixels = gdk_pixbuf_get_pixels(aPixbuf); hmm... this constness seems insufficient. shouldn't this use one that doesn't allow calling free on the pointer?

(In reply to comment #14) > (From update of attachment 211514 [details] [diff] [review] [edit]) > nsIconDecoder.cpp > > + PRInt32 w = *(data++); > + PRInt32 h = *(data++); > + PRInt32 alphaBits = *(data++); > > hm, why signed? Because that's what they were before, and that's what the functions they're used for take? But I'll make alphaBits a PRUint8.

Comment on attachment 211527 [details] [diff] [review] cache the GnomeIconTheme nsIconChannel.cpp + char* name = gnome_icon_lookup(gIconTheme, NULL, spec.get(), NULL, &fileInfo, + type.get(), GNOME_ICON_LOOKUP_FLAGS_NONE, NULL); why not align the second line so that it lines up with the first parameter?

Addresses biesi's comments.

Addresses biesi's comments (wraps to 3 lines now, though).

Comment on attachment 211543 [details] [diff] [review] use the Icon decoder (should fix PNG and file I/O perf issues) In moz_gdk_pixbuf_to_channel, you could fuse the color and alpha passes just by maintaining color_out and alpha_out pointers. This would be slightly less code and probably slightly more efficient.

Addresses roc's comments

Both patches checked in to trunk.

The only icon-related performance issues in a jprof profile of this testcase are: 38979 0 77 nsIconChannel::Init(nsIURI*) 75 gtk_widget_render_icon 1 moz_gdk_pixbuf_to_channel(_GdkPixbuf*, nsIURI*, nsIChannel**) 1 ensure_stock_image_widget()

The only icon-related performance issues in a jprof profile of this testcase are: 14746 0 52 nsIconChannel::InitWithGnome(nsIMozIconURI*) 40 gnome_icon_lookup 7 gdk_pixbuf_new_from_file 5 gdk_pixbuf_scale_simple

Note: The for loop's for SetImageData and SetAlphaData are not needed, see bug 318699. The patch for that bug also fixes the potential leak in nsIconDecoder.

Fix checked in to MOZILLA_1_8_BRANCH.