Bug 216199 – Remove storage WorkQueue in NetworkProcess

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Sihui Liu]

...

[Sihui Liu]

Created attachment 408045 [details]
Patch

[youenn fablet]

Comment on attachment 408045 [details]
Patch

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

> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:544
> +void IDBServer::collectOriginsForVersion(const String& versionPath, HashSet<WebCore::SecurityOriginData>& securityOrigins) const

Can we make it a static function instead of a method?

> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:69
> +        LockHolder locker(m_server->lock());

Shouldn't we lock in getOrigins instead?

> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1560
> +                callbackAggregator->m_websiteData.entries.append({ origins.takeAny(), WebsiteDataType::IndexedDBDatabases, 0 });

It seems we could return a Vector.
We added an optimisation for Vector::isolatedCopy() const && which would be nice as well to use.

[Sihui Liu]

(In reply to youenn fablet from comment #2)
> Comment on attachment 408045 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=408045&action=review
> 
> > Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:544
> > +void IDBServer::collectOriginsForVersion(const String& versionPath, HashSet<WebCore::SecurityOriginData>& securityOrigins) const
> 
> Can we make it a static function instead of a method?
> 
Sure.

> > Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:69
> > +        LockHolder locker(m_server->lock());
> 
> Shouldn't we lock in getOrigins instead?
> 
No, we hold this lock when performing task on IDB thread; so if we acquire this lock on the main thread, background thread will be stopped.

> > Source/WebKit/NetworkProcess/NetworkProcess.cpp:1560
> > +                callbackAggregator->m_websiteData.entries.append({ origins.takeAny(), WebsiteDataType::IndexedDBDatabases, 0 });
> 
> It seems we could return a Vector.
> We added an optimisation for Vector::isolatedCopy() const && which would be
> nice as well to use.

Where do you suggest using Vector? You mean appendVector() here?

[youenn fablet]

Comment on attachment 408045 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:69
>>> +        LockHolder locker(m_server->lock());
>> 
>> Shouldn't we lock in getOrigins instead?
> 
> No, we hold this lock when performing task on IDB thread; so if we acquire this lock on the main thread, background thread will be stopped.

I meant in IDBServer::getOrigins, not WebIDBServer::getOrigins.
You are calling m_server->lock() and then m_server->getOrigins(), it seems it could be optimised

>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1560
>>> +                callbackAggregator->m_websiteData.entries.append({ origins.takeAny(), WebsiteDataType::IndexedDBDatabases, 0 });
>> 
>> It seems we could return a Vector.
>> We added an optimisation for Vector::isolatedCopy() const && which would be nice as well to use.
> 
> Where do you suggest using Vector? You mean appendVector() here?

If getOrigins returns a Vector, we could use the Vector optimised of crossThreadCopy() and potentially use map here.
We might still need a HashSet to remove duplicates.
Recreating a HashSet through crossThreadCopy() is probably less efficient than a Vector created from a HashSet.
Or maybe we could try to optimise HashSet isolatedCopy in cases where all entries are already isolated.

[Sihui Liu]

Comment on attachment 408045 [details]
Patch

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

>>>> Source/WebKit/NetworkProcess/IndexedDB/WebIDBServer.cpp:69
>>>> +        LockHolder locker(m_server->lock());
>>> 
>>> Shouldn't we lock in getOrigins instead?
>> 
>> No, we hold this lock when performing task on IDB thread; so if we acquire this lock on the main thread, background thread will be stopped.
> 
> I meant in IDBServer::getOrigins, not WebIDBServer::getOrigins.
> You are calling m_server->lock() and then m_server->getOrigins(), it seems it could be optimised

I see, yeah, that makes sense, but this is how we do all the tasks right now: holding lock in WebIDBServer and verify lock is held in IDBServer, so I will keep it this way for now.

>>>> Source/WebKit/NetworkProcess/NetworkProcess.cpp:1560
>>>> +                callbackAggregator->m_websiteData.entries.append({ origins.takeAny(), WebsiteDataType::IndexedDBDatabases, 0 });
>>> 
>>> It seems we could return a Vector.
>>> We added an optimisation for Vector::isolatedCopy() const && which would be nice as well to use.
>> 
>> Where do you suggest using Vector? You mean appendVector() here?
> 
> If getOrigins returns a Vector, we could use the Vector optimised of crossThreadCopy() and potentially use map here.
> We might still need a HashSet to remove duplicates.
> Recreating a HashSet through crossThreadCopy() is probably less efficient than a Vector created from a HashSet.
> Or maybe we could try to optimise HashSet isolatedCopy in cases where all entries are already isolated.

I see, so you suggest that IDBServer::getOrigins() returns Vector and WebIDBServer::getOrigins makes a crossThreadCopy of the Vector? 
In NetworkProcess, we have filterForRegistrableDomains that takes HashSet, so seems it's more convenient to use HashSet there and as you said, maybe optimize isolatedCopy of HashSet.

[Sihui Liu]

Created attachment 408253 [details]
Patch for landing

[EWS]

Committed r266742: <https://trac.webkit.org/changeset/266742>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 408253 [details].

[Radar WebKit Bug Importer]

<rdar://problem/68524066>