IndexedDB: perform IDBServer work only on background thread

• Component: New Bugs
• Status: RESOLVED FIXED

[Sihui Liu]

For better performance

[Sihui Liu]

Created attachment 382489 [details]
WIP

[Sihui Liu]

Created attachment 382629 [details]
Patch

[Geoffrey Garen]

Comment on attachment 382629 [details]
Patch

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

> Source/WTF/ChangeLog:9
> +        Introduce a class ThreadMessageReceiver that handles IPC message on its own thread. It is similar to 
> +        WorkQueueMessageReceiver, but WorkQueue does not guarantee messages would be handled on the same thread.

I think this is the right design, and big improvement for performance, and just our ability to read the code in a straight-line way, rather than being split up between "perform" and "didPerform" messages.

> Source/WebCore/ChangeLog:26
> +        As we will need a different implementation for quota management of IDB after this change, quota management is
> +        temporarily removed.

We'll need to figure out how to re-integrate quota checking before we land this, to avoid test regressions.

> Source/WebCore/Modules/indexeddb/server/SQLiteIDBBackingStore.cpp:806
> +    // We use one serial WorkQueue for IDB work, which makes sure access to database is serialized,
> +    // but WorkQueue is not bound to one specific thread.
> +    m_sqliteDB->disableThreadingChecks();

Let's remove this, now that we're using an explicit thread.

> Source/WebKit/Platform/IPC/Connection.cpp:1022
> +    std::lock_guard<Lock> lock(m_threadMessageReceiversMutex);
> +    auto it = m_threadMessageReceivers.find(message->messageReceiverName());
> +    if (it != m_threadMessageReceivers.end()) {
> +        it->value->dispatchToThread([protectedThis = makeRef(*this), threadMessageReceiver = it->value, decoder = WTFMove(message)]() mutable {
> +            protectedThis->dispatchThreadMessageReceiverMessage(*threadMessageReceiver, *decoder);
> +        });
> +        return true;
> +    }
> +    return false;

I'd suggest putting the ThreadMessageReceiver in a local variable and then dropping the lock before calling dispatchToThread. That way, the ThreadMessageReceiver, and the code it calls into, doesn't need to worry about deadlocking if they call back into CoreIPC.

[Radar WebKit Bug Importer]

<rdar://problem/56908496>

[Sihui Liu]

Created attachment 384839 [details]
Patch

[Sihui Liu]

Created attachment 384842 [details]
Patch

[Sihui Liu]

Created attachment 384844 [details]
Patch

[Sihui Liu]

Created attachment 384857 [details]
Patch

[Brady Eidson]

Comment on attachment 384857 [details]
Patch

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

Everywhere an ASSERT(isMainThread()) is removed because that work is now done on the database thread, please change to ASSERT(isDatabaseThread()) (making isDatabaseThread() if it doesn't exist)

In general I'm concerned about how this change overhauls threading assumptions that had been built in to IDB since the rewrite.

I would suggest cutting as much out of this patch as possible to make it easier to review (e.g. moving IDBServer from WebCore to WebKitLegacy is not critical) and also see if we can find a way to do this in multiple steps.

We need to have a good strategy to make sure we don't introduce threading mistakes like the one mentioned further down.

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:634
>      for (auto& database : m_allUniqueIDBDatabases)
> -        database.resume();
> +        database.abortActiveTransactions();
>  }

This is one example of a threading error that was missed; It iterates over the set m_allUniqueIDBDatabases, but that set is also mutated by UniqueIDBDatabases on the database thread simultaneously. These happen with no locking.

[Sihui Liu]

Created attachment 384873 [details]
Patch

[Sihui Liu]

Created attachment 384986 [details]
WIP_reduced

[Sihui Liu]

Created attachment 385032 [details]
Patch

[Sihui Liu]

Created attachment 385033 [details]
Patch

[Sihui Liu]

Created attachment 385034 [details]
Patch

[Sihui Liu]

Created attachment 385038 [details]
Patch

[Sihui Liu]

Created attachment 385042 [details]
Patch

[Sihui Liu]

Created attachment 385045 [details]
Patch

[Brady Eidson]

Comment on attachment 385045 [details]
Patch

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

I'm taking a break.

> Source/WebCore/Modules/indexeddb/client/IDBConnectionToServer.cpp:80
>      if (m_serverConnectionIsValid)
> -        m_delegate->deleteDatabase(request);
> +    m_delegate->deleteDatabase(request);

These changes all seem unintentional?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:57
> +

We can assert the the directory path isSafeToSendToAnotherThread() (e.g. - it was isolated copied on the way in)

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:-120
> -    ASSERT(isMainThread());

No need to remove this one, really?

It's only caller today has an ASSERT, but what if another caller comes along tomorrow?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-132
> -    ASSERT(isMainThread());

I think we should change this instead of remove it... and also add one to the constructor.

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:863
> +    // FIXME: prefetch cursor.

Weird to see a FIXME added with little context.

IRL you told me it was related to posting tasks, timers, etc, and I suggested "RunLoop::current().dispath()" - Is that relevant to resolve this?

Or maybe we need a task for the prefetch to be put in the set or operations handled by handleDatabaseRequests()?

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:93
> +void WebIDBServer::postIDBTask(Function<void()>&& task)
> +{
> +    postTask(CrossThreadTask(WTFMove(task), m_suspendLock));
> +}

Main thread assertions! / RunLoop::isMain() assertions!

> Source/WebKitLegacy/Storage/InProcessIDBServer.cpp:-81
> -    ASSERT(isMainThread());

If InProcessIDBServer is constructed on the main thread, it should be destroyed on the main thread.

So this assertion should be in place.

> Source/WebKitLegacy/Storage/InProcessIDBServer.cpp:-115
> +void InProcessIDBServer::deleteDatabase(const WebCore::IDBRequestData& requestData)
>  {
> -    RunLoop::current().dispatch([this, protectedThis = makeRef(*this), requestData] {

Let's add an assertion saying this is only called on the main thread (and other methods like it)

> Source/WebKitLegacy/Storage/InProcessIDBServer.cpp:130
> +    dispatchTask([this, protectedThis = makeRef(*this), requestData = requestData.isolatedCopy()] {
> +        LockHolder locker(m_suspendLock);
>          m_server->deleteDatabase(requestData);
>      });

It's really weird to me that somebody else owns the lock, and a reference to it is passed in to IDBServer.

We're not protecting access outside the IDBServer methods (such as assignment to m_server), rather we are protecting thread coherency of the server functions/data itself.

I think IDBServer should have its own locker - with a public accessor - and this could be:
LockHolder locker(m_server->lock());

In addition to making more sense (to me) with regards to the role of the lock, I think it makes it much harder to accidentally make future mistakes.

E.g. In this patch, IDBServer has a Lock& m_lock, which means within the context of the IDBServer, the lifetime of the Lock is not guaranteed.
In this patch that's fine because the owner of the IDBServer also owns the Lock, but that is happenstance and could change in a future refactor.

> Source/WebKitLegacy/Storage/InProcessIDBServer.cpp:-122
>  void InProcessIDBServer::didDeleteDatabase(const IDBResultData& resultData)
>  {
> -    RunLoop::current().dispatch([this, protectedThis = makeRef(*this), resultData] {

Same command as above, but assert it's the background thread.

> Source/WebKitLegacy/Storage/InProcessIDBServer.h:132
> +    std::unique_ptr<WebCore::IDBServer::IDBServer> m_server;

WebIDBServer calls this m_idbServer instead of m_server.

Pick one and be consistent?

> Source/WebKitLegacy/Storage/InProcessIDBServer.h:139
> +    Lock m_suspendLock;

Let's not call this suspendLock.

Yes, the reason we have to do anything on the main thread is suspension, but the lock is not about suspension - It's about thread safety.

(I'd have just gone with m_lock)

[Sihui Liu]

Created attachment 385205 [details]
Patch

[Sihui Liu]

Created attachment 385206 [details]
Patch

[Brady Eidson]

Comment on attachment 385206 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:147
> +    m_pendingDatabaseRequests.add(ServerOpenDBRequest::create(connection, requestData));

See other comment about the fact that this set's name should clearly call out that it is a set of "OpenDBRequest" objects ^^^

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:-1687
> -    ASSERT(isMainThread());

What thread does this run on?
It should be exactly one, and have an assertion to that effect

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.h:156
> -    ListHashSet<RefPtr<ServerOpenDBRequest>> m_pendingOpenDBRequests;
> -    RefPtr<ServerOpenDBRequest> m_currentOpenDBRequest;
> +    ListHashSet<RefPtr<ServerOpenDBRequest>> m_pendingDatabaseRequests;
> +    RefPtr<ServerOpenDBRequest> m_currentDatabaseRequest;

I think renaming these makes it harder to understand what these objects are.

They *are* proxies of "IDBOpenDBRequests", so giving them a more generic name loses that meaning.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBConnectionToClient.cpp:60
> -    send(Messages::WebIDBConnectionToServer::DidDeleteDatabase(resultData));
> +    m_connection->send(Messages::WebIDBConnectionToServer::DidDeleteDatabase(resultData), 0);

Add a send() to WebIDBConnectionToClient that does this repeated boilerplate, and then many of these don't have to change.

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:370
> +    m_server->getAllDatabaseNames(idbConnection->identifier(), topOrigin, openingOrigin, callbackID);

No lock?

[Sihui Liu]

Per Brady's comment offline, I will try breaking down this big patch in to smaller ones so it's easier to review and less error-prone.

[Sihui Liu]

Created attachment 385910 [details]
Patch

[Alex Christensen]

Comment on attachment 385910 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:88
> -    WeakPtr<IDBConnectionToClientDelegate> m_delegate;
> +    IDBConnectionToClientDelegate* m_delegate;

We probably shouldn't be moving from smart pointers to raw pointers.  Why was this change made?

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:689
> +    // Release lock because space requesting could be blocked.
> +    m_lock.unlock();
> +    result = m_spaceRequester(origin, taskSize);
> +    m_lock.lock();

This kind of pattern can lead to interesting bugs if something else assumes this is done without anything being able to sneak in.  Could we do this some other way?

> Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:296
> +    // FIXME: remove this.

Why can't we do this in the same patch?

[Sihui Liu]

(In reply to Alex Christensen from comment #24)
> Comment on attachment 385910 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=385910&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/IDBConnectionToClient.h:88
> > -    WeakPtr<IDBConnectionToClientDelegate> m_delegate;
> > +    IDBConnectionToClientDelegate* m_delegate;
> 
> We probably shouldn't be moving from smart pointers to raw pointers.  Why
> was this change made?
> 

This change is not so relevant. I will revert this.

> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:689
> > +    // Release lock because space requesting could be blocked.
> > +    m_lock.unlock();
> > +    result = m_spaceRequester(origin, taskSize);
> > +    m_lock.lock();
> 
> This kind of pattern can lead to interesting bugs if something else assumes
> this is done without anything being able to sneak in.  Could we do this some
> other way?
> 
This is probably the easiest way to implement suspension for IDB. The purpose of releasing lock is to let suspension happen on main thread while database thread is blocked on space request.

> > Source/WebCore/Modules/indexeddb/server/UniqueIDBDatabase.cpp:296
> > +    // FIXME: remove this.
> 
> Why can't we do this in the same patch?

I will remove this before committing this patch. This is kept here for easier review with the diff tool (it does not recognize correct function changed).

[Alex Christensen]

Comment on attachment 385910 [details]
Patch

Ok, r=me when those are addressed.

[Sihui Liu]

Created attachment 386073 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 386073 [details]
Patch for landing

Clearing flags on attachment: 386073

Committed r253740: <https://trac.webkit.org/changeset/253740>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.