Bug 30941 - Refactor DatabaseTask in preparation for removing threadsafe refcounting from it
: Refactor DatabaseTask in preparation for removing threadsafe refcounting from it
Product: WebKit
Classification: Unclassified
Component: WebCore JavaScript
: 528+ (Nightly build)
: PC Mac OS X 10.5
: P2 Normal
Assigned To: Dmitry Titov
Depends on:
Blocks: 30612
  Show dependency treegraph
Reported: 2009-10-29 18:40 PDT by Dmitry Titov
Modified: 2009-10-30 17:21 PDT (History)
3 users (show)

See Also:

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

Note You need to log in before you can comment on or make changes to this bug.
Description Dmitry Titov 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.
Comment 1 Dmitry Titov 2009-10-29 19:02:44 PDT
Created attachment 42169 [details]
Proposed patch.
Comment 2 Michael Nordman 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.
Comment 3 Michael Nordman 2009-10-30 10:54:40 PDT

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 Dmitry Titov 2009-10-30 11:09:03 PDT
Created attachment 42218 [details]
Updated patch.

Updated. All Michael's comments make perfect sense. Thanks!
Comment 5 David Levin 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.
Comment 6 Dmitry Titov 2009-10-30 17:21:20 PDT
Landed: http://trac.webkit.org/changeset/50360