Today was one of those days when I realized how little I know about programming.
We liberally use boost::algorithm::iequals for case-insensitive string comparison, as well as occasional calls to istarts_with, iends_with, to_lower, etc. Unbeknownst to me previously, these things check the "locale" for information about how to do upper/lower case conversion, and if you don't pass an explicit locale to them, they will call std::locale() for it, which itself will actually lock a mutex in order to thread-safely discover the current locale. Any time texture tiles are requested from a different MIP level than the last tile read from that file, does a seek_subimage, which can trigger a new read of image header info for that subimage and creation of a new ImageSpec. That, it turns out, makes a substantial number of calls to iequals. When heavily threaded, then, the texture system can become heavily bottlenecked on those mutex locks, all because of innocent-looking iequals. Frankly, I have not noticed this on Linux or OSX (I often run with many threads on Linux, but rarely on OSX, so I'm less confident about that one), but reports of bad TextureSystem performance on WIndows have dogged us for a long time, and it looks like this is a major culprit. So, this patch creates Strutil wrappers for iequals, istarts_with, iens_with, to_upper, and to_lower. Their implementations use a static local to pass to the boost routines. This should completely avoid the mutex locking behavior as well as be thread-safe, though it will be immune to app-triggered changes in locale information that happen after OIIO's static locale is constructed (but I don't think that really matters for the way we use it, and if it does, we can fix it at that point). It's such a critical performance issue for some people, that I'm eager to backport it to both 1.0 and 0.10, so I've broken this up into several individual commits in order to make that merge a little easier to do with the files I anticipate for merge conflicts isolated, hopefully. After this is merged, I'll also look into ways to avoid doing so much work when switching MIPmap levels, but that might require some minor API or behavior changes, so I'm less confident about being able to safely backport it. You can merge this Pull Request by running: git pull https://github.com/lgritz/oiio lg-locale Or you can view, comment on it, or merge it online at: https://github.com/OpenImageIO/oiio/pull/255 -- Commit Summary -- * Strutil wrappers for iequals, istarts_with, iends_with, to_lower, to_upper. * Use new Strutil wrappers to avoid mutexed locale queries * Use new Strutil wrappers to avoid mutexed locale queries * Use new Strutil wrappers to avoid mutexed locale queries -- File Changes -- M src/bmp.imageio/bmpoutput.cpp (8) M src/dpx.imageio/dpxoutput.cpp (84) M src/ico.imageio/icooutput.cpp (5) M src/include/strutil.h (42) M src/include/ustring.h (14) M src/iprocess/iprocess.cpp (4) M src/iv/imageviewer.cpp (5) M src/iv/ivimage.cpp (2) M src/jpeg.imageio/jpegoutput.cpp (5) M src/libOpenImageIO/color_ocio.cpp (26) M src/libOpenImageIO/formatspec.cpp (7) M src/libOpenImageIO/imageioplugin.cpp (9) M src/libOpenImageIO/xmp.cpp (6) M src/libutil/strutil.cpp (64) M src/maketx/maketx.cpp (6) M src/oiiotool/oiiotool.cpp (13) M src/openexr.imageio/exrinput.cpp (24) M src/openexr.imageio/exroutput.cpp (62) M src/png.imageio/png_pvt.h (50) M src/png.imageio/pnginput.cpp (2) M src/png.imageio/pngoutput.cpp (3) M src/rla.imageio/rlainput.cpp (5) M src/rla.imageio/rlaoutput.cpp (8) M src/targa.imageio/targaoutput.cpp (6) M src/tiff.imageio/tiffoutput.cpp (63) -- Patch Links -- https://github.com/OpenImageIO/oiio/pull/255.patch https://github.com/OpenImageIO/oiio/pull/255.diff --- Reply to this email directly or view it on GitHub: https://github.com/OpenImageIO/oiio/pull/255 _______________________________________________ Oiio-dev mailing list [email protected] http://lists.openimageio.org/listinfo.cgi/oiio-dev-openimageio.org
