...
Created attachment 408045 [details] Patch
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.
(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?
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.
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.
Created attachment 408253 [details] Patch for landing
Committed r266742: <https://trac.webkit.org/changeset/266742> All reviewed patches have been landed. Closing bug and clearing flags on attachment 408253 [details].
<rdar://problem/68524066>