On Mon, Feb 01, 2010 at 04:39:11PM +0000, Robert Osfield wrote: > Hi David, > > I've just started reviewing your submission. The first things that > jumps out is that changing to a multiset makes the assumption that the > once a FileRequest is inserted it doesn't change it's own sort value. > This is not valid assumption as the design of database paging scheme > is that the priority of each request can change from frame to frame as > it come more into, or goes out further out range - making it more or > less important to load. The change you've proposed would break this > property so isn't acceptable.
The changes I made accounted for the changing sort variables issue by removing it from the multiset and inserting it again with the updated values. struct DatabaseRequest is a protected member of osgDB::DatabasePager, so it's only accessible by DatabasePager and derived classes. Just the same I did a global search for the variables used in sorting, which are _timestampLastRequest and _priorityLastRequest, and the only place they are assigned is the DatabasePager::requestNodeFile, so the semantics are preserved. > The race condition and implementation of locks/unlocks I haven't yet > got my head around so I will continue looking at this. One quick > feedback I can provide is that it's not generally safe programming to > lock() and then unlock() mutexes in conditional blocks. It would be > very very easy for a minor code change to end up with locking and > never unlocking or attempting to unlock a mutex that hasn't been > locked. The ScopedLock is used throughout the OSG code base to > prevent these mistakes from creeping into the code and is also > exception safe. I have little doubt that the better way to protect > things would be via a ScopedLock, even if these means restructuring > code to enable this. A ScopedLock only works if you know what lock to lock in the first place. This race condition exists because the requestNodeFile thread gets a pointer to the lock while another thread can (and does) modify that pointer meaning that the requestNodeFile was left holding a lock to the wrong lock now that the request moved to a different queue by the other thread. I wasn't seeing any way to use the ScopedLock without grabbing all four locks for _fileRequestQueue, _httpRequestQueue, _dataToCompileList, and _dataToMergeList because you don't know where the request will be, and besides performance grabbing all four locks involves the risk of lock ordering deadlocks. > One really surprising aspect to your post is that you discuss 10,000 > to 50,000 DatabaseRequests being generated in your application. This > is several orders of magnitude times greater than I'd ever expect for > an application. Something has to be seriously wrong in the database > or the app/pager for this to happen. This really does warrant further > investigation/explanation, the DatabasePager has never been designed > to handle this level of requests, and neither do I expect it ever to > be used with this level of requests, the hardware, OS and software > will never been able to handle tens of thousands of file requests per > second. That's kind of the point. It isn't, they are just piling up as the DatabasePager isn't able to keep up, and as they pile up the sort operation dominates the processing and it keeps getting further behind. With the vector it takes a exponentially long amount of time to make it through the list when the demand decreases and it can catch up, with the multiset it sometimes catches up with a hickup from the request side. Without looking at the requests I expect the same tiles are being requested over and over without a databaseRequest pointer to the existing request, and the old can be expired. So they are probably just duplicates, and that problem should be addressed independently of sorting the list for each request the database pager pulls off. > How are you able to generate this number of requests? What type of > database are you working with? What type of application? Just osgviewer and the public earth.ive dataset, zoom in and give it a spin like I described. It might even be easier to see with an empty cache. > > Note: requires StatsHandler in osgviewer to see the request queue. > > viewer.addEventHandler(new osgViewer::StatsHandler); > > > > The whole earth blue marble can be used, zoom into the highest level > > of detail and give it a good spin. ?If it isn't getting behind load up > > the computer with other tasks. > > http://www.openscenegraph.org/data/earth_bayarea/earth.ive -- David Fries <[email protected]> http://fries.net/~david/ (PGP encryption key available) _______________________________________________ osg-submissions mailing list [email protected] http://lists.openscenegraph.org/listinfo.cgi/osg-submissions-openscenegraph.org
