GNOME Bugzilla – Bug 69152
PATCH: make g_convert cache iconv descriptors
Last modified: 2011-02-18 15:47:19 UTC
On systems like Solaris (and even Linux?), iconv_open() can be fairly slow if due to having to dlopen a module for each charset involved in the conversion. The attached patch is an attempt at minimizing the overhead of iconv_open by caching iconv descriptors. In writing this patch I discovered a bug (for which I will make a separate bug report in a few moments) that didn't close an opened converter in g_convert_with_fallback() when the code fails to get utf8 text. The attached patch also fixes that...
Created attachment 6449 [details] [review] gconvert.patch
Created attachment 6465 [details] [review] updated to apply cleanly to latest cvs (after fix for bug #69153 was applied)
This is very nice code, and looks like it should do the job well. Still, I can't help but thinking that it is just a bit over-engineered... it's a fair chunk of code. The one suggestion I would make for simplification, is that I think we can assume: "If one converter for a encoding pair is loaded, it will be cheap to load a second one" With that in mind, I don't think it would hurt to remove the code handling multiple open converters for the same key. Other comments: - I'd ditch the g_atexit() cleanup handler or at least #ifdef it out. atexit() handlers are a scarce resource on some systems, and if we don't do this anywhere else in GLib, I don't see a point in doing it here. (ref. my well-known bias about proper memory leak debugging techniques ;-) - If written from scratch, I'd prefer to see the code use the GLib data structures ... GList and GQeuue; the cache isn't big enough to make the overhead of separate list nodes / structures important. It's probably not worth reworking at this point.
Okay, I've had another go at it. This time we only ever cache a single iconv_t descriptor for any conversion type. I've also modified the code to use GList rather than generic list. also, I found several bugs in the last patch so if it is decided that you prefer the old code, let me know and I'll post the fixed up version of that.
Created attachment 6512 [details] [review] new proposed patch
Looks fine, please commit.
just committed to cvs.