Bug 125127 – Indexed Database work should be done on a non-main queue

• Status: RESOLVED
• Resolution: FIXED
• Component: WebKit2

[Brady Eidson]

IDB database work should be done on a non-main queue

In other words, i/o should not be on the main thread.

[Brady Eidson]

Created attachment 218253 [details]
Patch v1

This patch still doesn't actually do any useful work (database i/o), but it does establish all of the necessary infrastructure for a Unique IDB Database to have a background queue of requests to service.

[WebKit Commit Bot]

Attachment 218253 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/Shared/AsyncRequest.cpp', u'Source/WebKit2/Shared/AsyncRequest.h', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp']" exit_code: 1
ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:102:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:42:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:44:  Missing spaces around &&  [whitespace/operators] [3]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:75:  Missing spaces around &&  [whitespace/operators] [3]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.h:97:  Missing spaces around &&  [whitespace/operators] [3]
ERROR: Source/WebKit2/Shared/AsyncRequest.cpp:41:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WebKit2/Shared/AsyncRequest.cpp:52:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 11 in 9 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Brady Eidson]

(In reply to comment #2)
> Attachment 218253 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WebKit2/ChangeLog', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.cpp', u'Source/WebKit2/DatabaseProcess/DatabaseProcess.h', u'Source/WebKit2/DatabaseProcess/IndexedDB/DatabaseProcessIDBConnection.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp', u'Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h', u'Source/WebKit2/Shared/AsyncRequest.cpp', u'Source/WebKit2/Shared/AsyncRequest.h', u'Source/WebKit2/WebProcess/Databases/IndexedDB/WebIDBServerConnection.cpp']" exit_code: 1
> ERROR: Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:102:  Place brace on its own line for function definitions.  [whitespace/braces] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:42:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:44:  Missing spaces around &&  [whitespace/operators] [3]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:47:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:51:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:65:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:75:  Missing spaces around &&  [whitespace/operators] [3]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:82:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.h:97:  Missing spaces around &&  [whitespace/operators] [3]
> ERROR: Source/WebKit2/Shared/AsyncRequest.cpp:41:  Extra space before ( in function call  [whitespace/parens] [4]
> ERROR: Source/WebKit2/Shared/AsyncRequest.cpp:52:  Extra space before ( in function call  [whitespace/parens] [4]
> Total errors found: 11 in 9 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

These errors are actually all false positives, bugs exist on some, and I'll make sure they exist on the other(S)

[Darin Adler]

Comment on attachment 218253 [details]
Patch v1

View in context: https://bugs.webkit.org/attachment.cgi?id=218253&action=review

> Source/WebKit2/DatabaseProcess/DatabaseProcess.h:77
> +    RefPtr<WorkQueue> m_queue;

Should use Ref for this instead of RefPtr.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:74
> +    m_processDatabaseRequestQueueCalled = true;

I don’t understand how this ever gets set back to false. It seems like once we complete all there requests in the queue a single time we will no longer handle any requests.

The flaw seems to be in the name too. It just says that this function was “called”, which isn’t really what we want to know. I think it should be called m_processingDatabaseQueueRequests or something like that.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:105
> +            IDBDatabaseMetadata metadata;
> +            completionCallback(false, metadata);

Could write this as a one-liner:

    completionCallback(false, IDBDatabaseMetadata());

Also, constant false is unclear here at this call site.

> Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:74
> +    void enqueueDatabaseQueueRequest(PassRefPtr<AsyncRequest>);

I think this should be PassRef rather than PassRefPtr since it’s never null. Been a while so I may have forgotten what the right type is.

> Source/WebKit2/Shared/AsyncRequest.h:47
> +    AsyncRequest(std::function<void ()> abortHandler);

Should mark this explicit.

> Source/WebKit2/Shared/AsyncRequest.h:83
> +        : AsyncRequest(abortHandler)

Why no std::move here?

[Brady Eidson]

(In reply to comment #4)
> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:74
> > +    m_processDatabaseRequestQueueCalled = true;
> 
> I don’t understand how this ever gets set back to false. It seems like once we complete all there requests in the queue a single time we will no longer handle any requests.
> 
> The flaw seems to be in the name too. It just says that this function was “called”, which isn’t really what we want to know. I think it should be called m_processingDatabaseQueueRequests or something like that.

I had one line that wasn't staged and therefore didn't make it into the patch, and that was reseting this flag inside processDatabaseRequestQueue().  Last minute debugging fail  :(

I like your naming suggestion, and will adopt it.


> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.cpp:105
> > +            IDBDatabaseMetadata metadata;
> > +            completionCallback(false, metadata);
> 
> Could write this as a one-liner:
> 
>     completionCallback(false, IDBDatabaseMetadata());
> 
> Also, constant false is unclear here at this call site.

I hate the IDB interface layer that we're currently stuck with, and giving it a once-over for clarity is in the future.

Right now, I'll add a brief comment.

> 
> > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:74
> > +    void enqueueDatabaseQueueRequest(PassRefPtr<AsyncRequest>);
> 
> I think this should be PassRef rather than PassRefPtr since it’s never null. Been a while so I may have forgotten what the right type is.

Seems Ref/PassRef would be ideal.  I'll try it and see if we avoid some of the common Ref/PassRef traps (deleted c'tors/d'tors, stuff like that)

> > Source/WebKit2/Shared/AsyncRequest.h:47
> > +    AsyncRequest(std::function<void ()> abortHandler);
> 
> Should mark this explicit.

Yup.
 
> > Source/WebKit2/Shared/AsyncRequest.h:83
> > +        : AsyncRequest(abortHandler)
> 
> Why no std::move here?

std::move all the things!  (Will do).

[Brady Eidson]

(In reply to comment #5)
> (In reply to comment #4)
> > > Source/WebKit2/DatabaseProcess/IndexedDB/UniqueIDBDatabase.h:74
> > > +    void enqueueDatabaseQueueRequest(PassRefPtr<AsyncRequest>);
> > 
> > I think this should be PassRef rather than PassRefPtr since it’s never null. Been a while so I may have forgotten what the right type is.
> 
> Seems Ref/PassRef would be ideal.  I'll try it and see if we avoid some of the common Ref/PassRef traps (deleted c'tors/d'tors, stuff like that)

Definitely falls into common pitfalls with NONCOPYABLE and some of our container classes not handling Ref<> well (in this case Deque).

I agree this is conceptually correct, but getting it to work technically will be a bit out of the scope of this patch.

[Brady Eidson]

http://trac.webkit.org/changeset/160033