<div dir="ltr"><div><div><div><div><div><div>Jacek and Even,<br><br></div>I already made an improvement on the pthread version of this:<br><br><a href="https://github.com/flippmoke/gdal/blob/trunk/gdal/port/cpl_multiproc.cpp#L1135" target="_blank">https://github.com/flippmoke/gdal/blob/trunk/gdal/port/cpl_multiproc.cpp#L1135</a><br><br></div>I am sure we could do something similar to it to make an improvement in the non pthread version. <br><br></div>Adding at line:<br><br><a href="https://github.com/flippmoke/gdal/blob/trunk/gdal/port/cpl_multiproc.cpp#L165" target="_blank">https://github.com/flippmoke/gdal/blob/trunk/gdal/port/cpl_multiproc.cpp#L165</a><br><br></div>We could do something like this:<br><br></div><a href="https://gist.github.com/flippmoke/16cd26a0247422f25840" target="_blank">https://gist.github.com/flippmoke/16cd26a0247422f25840</a><br><br></div>Thanks,<br><br>Blake</div><div class="gmail_extra"><br><div class="gmail_quote">On Fri, Nov 28, 2014 at 11:49 AM, Even Rouault <span dir="ltr"><<a href="mailto:even.rouault@spatialys.com" target="_blank">even.rouault@spatialys.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">Jacek,<br>
<br>
Did you identify which call sites triggered hCOAMutex to be taken ? Ideally we<br>
should try to minimize them.<br>
For the sake of my curiosity, which method did you try to evaluate time spend<br>
in mutex sleeping ?<br>
Another improvement would be that instead of dynamically created mutexes, we<br>
could use statically created mutexes ( PTHREAD_RECURSIVE_MUTEX_INITIALIZER ).<br>
That would avoid that hCOAMutex to become the "BGL" (Big GDAL Lock :<br>
<a href="http://en.wikipedia.org/wiki/Giant_lock" target="_blank">http://en.wikipedia.org/wiki/Giant_lock</a>)<br>
Building GDAL with MUTEX_NONE and using it in a multi-threaded case is a<br>
potential risky game. This might work for some particular workloads & drivers,<br>
but not in the general case. For example even the GTiff driver needs a mutex to<br>
initialize libtiff/libgeotiff. See GTiffOneTimeInit() and there might be more<br>
subtle stuff elsewhere.<br>
But yes, I agree that an open flag to indicate per-dataset cache is an<br>
interesting track to explore.<br>
<br>
Even<br>
<br>
</span><div><div class="h5">> Blake, Even,<br>
> I played a bit with pull request #39. I enabled per dataset cache by setting<br>
> GDAL_DATASET_CACHING to YES. Awesome work!<br>
> Our application code opens file in GDAL and holds GDALDataset per thread.<br>
> Different threads can request same blocks but because cache should be per<br>
> dataset i expected to see very little contention at the expense of some of<br>
> the blocks being read multiple times.<br>
><br>
> To my surprise i saw that nothing changed in terms of time spent sleeping.<br>
> After some investigation i realized that the contention point is static mutex<br>
> hCOAMutex. Having noticed that i saw MUTEX_NONE ifdef there so i decided to<br>
> define it to effectively get rid of mutexes in GDAL.<br>
> Guess what happened after recompilation. Because we only ever access<br>
> GDALDataset from single dataset and the new GDALBlockManager is now per<br>
> GDALDataset everything so far* (with the exception of PAMDataset destruction<br>
> because it does not like null mutexes) works fine and scales so much better<br>
> with the number of threads.<br>
><br>
> *Tested reading only using GTiff driver 4 threads reading tiles smaller than<br>
> blocks, sometimes two threads can read data from the same block .<br>
><br>
> Do you see the same problem with hCOAMutex?<br>
> It would be good if i could say: give me per dataset cache, open this dataset<br>
> with no locking. Unless the global cache provides its operations with less<br>
> sleeping.<br>
><br>
> Regards.<br>
> Jacek Tomaka<br>
> ________________________________<br>
> Od: <a href="mailto:gdal-dev-bounces@lists.osgeo.org">gdal-dev-bounces@lists.osgeo.org</a> [<a href="mailto:gdal-dev-bounces@lists.osgeo.org">gdal-dev-bounces@lists.osgeo.org</a>] w<br>
> imieniu Blake Thompson [<a href="mailto:flippmoke@gmail.com">flippmoke@gmail.com</a>]<br>
</div></div>> Wys³ano: 29 sierpnia 2014 02:35<br>
<div class="HOEnZb"><div class="h5">> Do: Even Rouault<br>
> DW: <a href="mailto:gdal-dev@lists.osgeo.org">gdal-dev@lists.osgeo.org</a><br>
> Temat: Re: [gdal-dev] RFC 47 and Threading<br>
><br>
> Even and Andre,<br>
><br>
> > I want to start off by saying a big thanks to Blake for taking his time<br>
> > to tackle what can only be a very difficult problem.<br>
> > From what I can observe, the current discussion seems to be around the<br>
> > boundary of who should be responsible for ensuring thread safety around<br>
> > the block cache. The core of GDAL versus the individual drivers.<br>
><br>
> The core will necessarily have to know about thread-safety because the block<br>
> cache is there. The discussion is more whether the drivers must also<br>
> necessary<br>
> be thread aware, or if core mechanisms are sufficient to hide this detail to<br>
> the<br>
> drivers. And potentially offering to drivers a mechanism to deal themselves<br>
> with<br>
> thread-safety if they can have a more optimized implementation than the<br>
> default<br>
> one.<br>
><br>
> Agreed, most specifically how to hide the detail of the protection of the<br>
> pointer to the block cache's data that is passed through IReadBlock,<br>
> IWriteBlock, and IRasterIO.<br>
><br>
><br>
> > While I<br>
> > see why such a conversation is important, as far as I am concerned, the<br>
> > most important part should be how it affects users of GDAL at the<br>
> > interface level.<br>
><br>
> Agreed.<br>
><br>
> > That is, if an application that is threaded is trying<br>
> > to use GDAL, how does it ensure thread-safety? What you have to keep in<br>
> > mind is that having some parts of the library not thread-safe basically<br>
> > just pushes the mutexing/locking to the calling applications.<br>
><br>
> Not necessarily. That's what I suggested in my previous email : if the costs<br>
> of<br>
> the mutex are not too expensive for non-threaded usage, then the API could<br>
> systematically return thread-safe versions, that are potentially wrapped by<br>
> GDALDatasetThreadSafe<br>
><br>
> So in the scope of my RFC, I am not certain how we could have a thread safe<br>
> cache and a non-threadsafe cache in any simple manner. I know that you are<br>
> specifically talking about the possiblity of thread safe datasets, which I<br>
> feel is necessarily part of this discussion but wanted to separate the two.<br>
> If the thread-safe cache is too expensive I feel like that is a major issue<br>
> however, and I am doing my best to avoid any performance hits for this<br>
> change.<br>
><br>
><br>
> ><br>
> > Also, while it is important to document thread-safety limitations, might<br>
> > I suggest adding thread-safe related capabilities (TestCapability),<br>
> > especially if all drivers do not end-up having the same thread-safety<br>
> > constraints.<br>
><br>
> That might be a solution, although not ideal from a usability point of view<br>
> (if<br>
> we come with something more complex that non-thread-safe vs thread-safe, that<br>
> might be difficult to understand by users of GDAL API), and from a<br>
> GDAL-developer point of view as well (need to assess thread-safety for each<br>
> driver).<br>
> There can have subtelties : imagine that the VRT driver is made to be thread<br>
> safe, but uses sources of drivers that might be not thread safe...<br>
><br>
> ><br>
> > I personally do not see a GDALDatasetThreadSafe wrapper as adding much<br>
> > complexity. For instance, if you were to add a capability that indicates<br>
> > if a driver is inherently thread-safe, you could add a new open method<br>
> > to open a dataset in a thread-safe way with something like the following<br>
> > pseudocode:<br>
> ><br>
> > DataSet OpenThreadSafe(GDALOpenInfo openInfo)<br>
> > {<br>
> > DataSet dataSet = Open(openInfo);<br>
> ><br>
> > if (!dataSet.TestCapability(THREAD_SAFE))<br>
> > {<br>
> > dataSet = wrapWithThreadSafeWrapper(dataSet);<br>
> > }<br>
> ><br>
> > return dataSet;<br>
> > }<br>
><br>
> Yes, that's similar to what I imagined with my idea of GDAL_OF_THREADSAFE<br>
> open<br>
> flag.<br>
><br>
> On the topic of the thread safe wrapper, I have spent more time thinking<br>
> about it and I think this is probably the best solution to the problem of<br>
> making all Datasets read safe, and I am willing to champion another RFC to<br>
> implement this. However, the scope of this is even larger because it should<br>
> be required to work with the OGR datasets as well.<br>
><br>
> Thanks,<br>
><br>
> Blake<br>
><br>
<br>
<br>
</div></div><div class="HOEnZb"><div class="h5">--<br>
Spatialys - Geospatial professional services<br>
<a href="http://www.spatialys.com" target="_blank">http://www.spatialys.com</a><br>
</div></div></blockquote></div><br></div>