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