Bug 30941

Summary: Refactor DatabaseTask in preparation for removing threadsafe refcounting from it
Product: WebKit Reporter: Dmitry Titov <dimich>
Component: WebCore JavaScriptAssignee: Dmitry Titov <dimich>
Status: RESOLVED FIXED    
Severity: Normal CC: ap, levin, michaeln
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: PC   
OS: OS X 10.5   
Bug Depends on:    
Bug Blocks: 30612    
Attachments:
Description Flags
Proposed patch.
dimich: commit-queue-
Updated patch. levin: review+, dimich: commit-queue-

Dmitry Titov
Reported 2009-10-29 18:40:23 PDT
DatabaseTask contains lock inside itself so the main thread that posts the task for the database thread can wait of the task for the completion. Some operations on Database are synchronous and they are implemented this way. This change moves the lock out from the task so it can be created separately and used on the main thread. This removes the need to keep the pointer to the task beyond posting it into the MessageQueue. This is needed to make MessageQueue OwnPtr-like and removing the threadsafe refcounting from the DatabaseTask. Also, the queue of SQL transactions can be just Deque rather then MessageQueue - it doesn't use the waitForMessage functionality anyways.
Attachments
Proposed patch. (15.06 KB, patch)
2009-10-29 19:02 PDT, Dmitry Titov 		dimich: commit-queue-
DetailsFormatted DiffDiff
Updated patch. (15.24 KB, patch)
2009-10-30 11:09 PDT, Dmitry Titov 		levin: review+
dimich: commit-queue-
DetailsFormatted DiffDiff
Show Obsolete (1) View All Add attachment proposed patch, testcase, etc.
Dmitry Titov
Comment 1 2009-10-29 19:02:44 PDT
Created attachment 42169 [details] Proposed patch.
Michael Nordman
Comment 2 2009-10-30 09:55:15 PDT
generally, looks reasonable to me * Maybe ASSERT(synchronizer) in the ctors for the tasks that take references to variables on the callers stack in which the results will be stored. 38 DatabaseTaskSynchronizer::DatabaseTaskSynchronizer() 39 : m_taskCompleted(false) 40 { 41 m_synchronousMutex.set(new Mutex); 42 m_synchronousCondition.set(new ThreadCondition); 43 } I think these data members should be defined inline w/o heap allocating them, as there's no reason to construct this class unless its going to be used for sync'ing. As data members of the Task class, they were rarely used, so heap allocating independently in that rare case made sense.
Michael Nordman
Comment 3 2009-10-30 10:54:40 PDT
Also... class Database { bool m_isProcessingTasks; }; Maybe rename the new data member to m_isTransactionQueueEnabled. Yong Li has a patch out for review that uses a very similarly named data member (in the DatabaseThread class i think) for a very different purpose, m_isProcessingTask (singular and actually applies to any DatabaseTask). Renaming this one in the Database class would could avoid cognitive collisions with that. Also, I think m_isTransactionQueueEnabled is a more accurate name for this new data member irrespective of what's going on in DatabaseThread.
Dmitry Titov
Comment 4 2009-10-30 11:09:03 PDT
Created attachment 42218 [details] Updated patch. Updated. All Michael's comments make perfect sense. Thanks!
David Levin
Comment 5 2009-10-30 13:20:07 PDT
Comment on attachment 42218 [details] Updated patch. Just a few nits to address on check in. > diff --git a/WebCore/ChangeLog b/WebCore/ChangeLog > + (WebCore::DatabaseTask::DatabaseTask): Ctor takes DatabaseTaskSynchronizer which can be NULL. Use 0 instead of NULL (since NULL isn't used in the code). > diff --git a/WebCore/storage/Database.h b/WebCore/storage/Database.h > @@ -56,43 +78,19 @@ void DatabaseTask::performTask() > + if(m_synchronizer) add space after if. > diff --git a/WebCore/storage/DatabaseTask.h b/WebCore/storage/DatabaseTask.h > + static PassRefPtr<DatabaseOpenTask> create(Database* db, DatabaseTaskSynchronizer* synchronizer, ExceptionCode& code, bool& success) { brace position. > + static PassRefPtr<DatabaseTransactionTask> create(PassRefPtr<SQLTransaction> transaction) { brace position. > + static PassRefPtr<DatabaseTableNamesTask> create(Database* db, DatabaseTaskSynchronizer* synchronizer, Vector<String>& names) { brace position. There are several more functions with bad brace placement as well.
Dmitry Titov
Comment 6 2009-10-30 17:21:20 PDT
Landed: http://trac.webkit.org/changeset/50360
Note You need to log in before you can comment on or make changes to this bug.