Bug 30941 - Refactor DatabaseTask in preparation for removing threadsafe refcounting from it
: Refactor DatabaseTask in preparation for removing threadsafe refcounting from it
Status: RESOLVED FIXED
: WebKit
WebCore JavaScript
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To:
:
:
:
: 30612
  Show dependency treegraph
 
Reported: 2009-10-29 18:40 PST by
Modified: 2009-10-30 17:21 PST (History)


Attachments
Proposed patch. (15.06 KB, patch)
2009-10-29 19:02 PST, Dmitry Titov
dimich: commit‑queue-
Review Patch | Details | Formatted Diff | Diff
Updated patch. (15.24 KB, patch)
2009-10-30 11:09 PST, Dmitry Titov
levin: review+
dimich: commit‑queue-
Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2009-10-29 18:40:23 PST
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.
------- Comment #1 From 2009-10-29 19:02:44 PST -------
Created an attachment (id=42169) [details]
Proposed patch.
------- Comment #2 From 2009-10-30 09:55:15 PST -------
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.
------- Comment #3 From 2009-10-30 10:54:40 PST -------
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.
------- Comment #4 From 2009-10-30 11:09:03 PST -------
Created an attachment (id=42218) [details]
Updated patch.

Updated. All Michael's comments make perfect sense. Thanks!
------- Comment #5 From 2009-10-30 13:20:07 PST -------
(From update of attachment 42218 [details])
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.
------- Comment #6 From 2009-10-30 17:21:20 PST -------
Landed: http://trac.webkit.org/changeset/50360