corysanin/tubestation
1
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Bug 1965643 - IDB: Replace global array gFactoryOps with a linked list to improve factory operation handling; r=dom-storage-reviewers,aiunusov

Browse Source 
Replaces the global gFactoryOps array with a linked list to improve performance
during factory operation handling.

This change improves performance in scenarios where many database open/close
operations are initiated in parallel. The array previously used for tracking
active FactoryOp instances incurred linear-time removal overhead, which the
linked list avoids. Unlike a hash set, the linked list preserves insertion
order, which is required for correctness.

This is part of the ongoing effort to reduce jank caused by frequent IndexedDB
open/close activity, tracked in the associated meta bug.

Differential Revision: https://phabricator.services.mozilla.com/D248708
This commit is contained in:
Jan Varga
2025-05-16 07:42:50 +00:00
committed by jvarga@mozilla.com
parent 1de23dc2a6
commit 20ad00a0fe
1 changed files with 27 additions and 18 deletions
Download Patch File Download Diff File Expand all files Collapse all files

45
dom/indexedDB/ActorsParent.cpp
View File

@@ -120,7 +120,6 @@
#include "mozilla/dom/ipc/IdType.h" #include "mozilla/dom/ipc/IdType.h"
#include "mozilla/dom/quota/Assertions.h" #include "mozilla/dom/quota/Assertions.h"
#include "mozilla/dom/quota/CachingDatabaseConnection.h" #include "mozilla/dom/quota/CachingDatabaseConnection.h"
#include "mozilla/dom/quota/CheckedUnsafePtr.h"
#include "mozilla/dom/quota/Client.h" #include "mozilla/dom/quota/Client.h"
#include "mozilla/dom/quota/ClientDirectoryLock.h" #include "mozilla/dom/quota/ClientDirectoryLock.h"
#include "mozilla/dom/quota/ClientDirectoryLockHandle.h" #include "mozilla/dom/quota/ClientDirectoryLockHandle.h"
@@ -2931,9 +2930,8 @@ class VersionChangeTransaction final
const OpenCursorParams& aParams) override; const OpenCursorParams& aParams) override;
}; };
class FactoryOp class FactoryOp : public DatabaseOperationBase,
: public DatabaseOperationBase, public LinkedListElement<FactoryOp> {
public SupportsCheckedUnsafePtr<CheckIf<DiagnosticAssertEnabled>> {
public: public:
struct MaybeBlockedDatabaseInfo final { struct MaybeBlockedDatabaseInfo final {
SafeRefPtr<Database> mDatabase; SafeRefPtr<Database> mDatabase;
@@ -3110,6 +3108,7 @@ class FactoryOp
// MSVC 2010 fails to link for some reason if it is not inlined here... // MSVC 2010 fails to link for some reason if it is not inlined here...
MOZ_ASSERT_IF(OperationMayProceed(), MOZ_ASSERT_IF(OperationMayProceed(),
mState == State::Initial || mState == State::Completed); mState == State::Initial || mState == State::Completed);
MOZ_DIAGNOSTIC_ASSERT(!isInList());
} }
nsresult Open(); nsresult Open();
@@ -5878,7 +5877,13 @@ nsresult RemoveDatabaseFilesAndDirectory(nsIFile& aBaseDirectory,
// Counts the number of "live" Factory, FactoryOp and Database instances. // Counts the number of "live" Factory, FactoryOp and Database instances.
uint64_t gBusyCount = 0; uint64_t gBusyCount = 0;
using FactoryOpArray = nsTArray<CheckedUnsafePtr<FactoryOp>>; // We don't use LinkedList<CheckedUnsafePtr<FactoryOp>> because
// CheckedUnsafePtr is not suitable for use within LinkedList. While it's
// theoretically possible to adapt LinkedList to support it, doing so would
// introduce unnecessary overhead. Instead, we use a simpler and more
// efficient approach. Each FactoryOp instance asserts !isInList() in its
// destructor to catch dangling pointer issues.
using FactoryOpArray = LinkedList<FactoryOp>;
StaticAutoPtr<FactoryOpArray> gFactoryOps; StaticAutoPtr<FactoryOpArray> gFactoryOps;
@@ -5983,7 +5988,7 @@ void DecreaseBusyCount() {
gLiveDatabaseHashtable = nullptr; gLiveDatabaseHashtable = nullptr;
MOZ_ASSERT(gFactoryOps); MOZ_ASSERT(gFactoryOps);
MOZ_ASSERT(gFactoryOps->IsEmpty()); MOZ_ASSERT(gFactoryOps->isEmpty());
gFactoryOps = nullptr; gFactoryOps = nullptr;
#ifdef DEBUG #ifdef DEBUG
@@ -9085,7 +9090,7 @@ Factory::AllocPBackgroundIDBFactoryRequestParent(
} }
}(); }();
gFactoryOps->AppendElement(actor); gFactoryOps->insertBack(actor);
// Balanced in CleanupMetadata() which is/must always called by SendResults(). // Balanced in CleanupMetadata() which is/must always called by SendResults().
IncreaseBusyCount(); IncreaseBusyCount();
@@ -9152,7 +9157,7 @@ mozilla::ipc::IPCResult Factory::RecvGetDatabases(
aPersistenceType, aPrincipalInfo, aPersistenceType, aPrincipalInfo,
std::move(aResolve)); std::move(aResolve));
gFactoryOps->AppendElement(op); gFactoryOps->insertBack(op);
// Balanced in CleanupMetadata() which is/must always called by SendResults(). // Balanced in CleanupMetadata() which is/must always called by SendResults().
IncreaseBusyCount(); IncreaseBusyCount();
@@ -12605,7 +12610,7 @@ void QuotaClient::InitiateShutdown() {
} }
bool QuotaClient::IsShutdownCompleted() const { bool QuotaClient::IsShutdownCompleted() const {
return (!gFactoryOps || gFactoryOps->IsEmpty()) && return (!gFactoryOps || gFactoryOps->isEmpty()) &&
(!gLiveDatabaseHashtable || !gLiveDatabaseHashtable->Count()) && (!gLiveDatabaseHashtable || !gLiveDatabaseHashtable->Count()) &&
!mCurrentMaintenance && !DeleteFilesRunnable::IsDeletionPending(); !mCurrentMaintenance && !DeleteFilesRunnable::IsDeletionPending();
} }
@@ -12619,17 +12624,17 @@ nsCString QuotaClient::GetShutdownStatus() const {
nsCString data; nsCString data;
if (gFactoryOps && !gFactoryOps->IsEmpty()) { if (gFactoryOps && !gFactoryOps->isEmpty()) {
data.Append("FactoryOperations: "_ns + data.Append("FactoryOperations: "_ns +
IntToCString(static_cast<uint32_t>(gFactoryOps->Length())) + IntToCString(static_cast<uint32_t>(gFactoryOps->length())) +
" ("_ns); " ("_ns);
// XXX It might be confusing to remove duplicates here, as the actual list // XXX It might be confusing to remove duplicates here, as the actual list
// won't match the count then. // won't match the count then.
nsTHashSet<nsCString> ids; nsTHashSet<nsCString> ids;
std::transform(gFactoryOps->cbegin(), gFactoryOps->cend(), std::transform(gFactoryOps->begin(), gFactoryOps->end(), MakeInserter(ids),
MakeInserter(ids), [](const auto& factoryOp) { [](const FactoryOp* const factoryOp) {
MOZ_ASSERT(factoryOp); MOZ_ASSERT(factoryOp);
nsCString id; nsCString id;
@@ -13430,9 +13435,10 @@ nsresult Maintenance::BeginDatabaseMaintenance() {
public: public:
static bool IsSafeToRunMaintenance(const nsAString& aDatabasePath) { static bool IsSafeToRunMaintenance(const nsAString& aDatabasePath) {
if (gFactoryOps) { if (gFactoryOps) {
for (uint32_t index = gFactoryOps->Length(); index > 0; index--) { // XXX LinkedList should support reverse iteration via rbegin() and
CheckedUnsafePtr<FactoryOp>& existingOp = (*gFactoryOps)[index - 1]; // rend(), see bug 1964967.
for (const FactoryOp* existingOp = gFactoryOps->getLast(); existingOp;
existingOp = existingOp->getPrevious()) {
if (existingOp->DatabaseNameRef().isNothing()) { if (existingOp->DatabaseNameRef().isNothing()) {
return false; return false;
} }
@@ -14913,7 +14919,10 @@ nsresult FactoryOp::DirectoryWorkDone() {
bool foundThis = false; bool foundThis = false;
bool blocked = false; bool blocked = false;
for (const auto& existingOp : Reversed(*gFactoryOps)) { // XXX LinkedList should support reverse iteration via rbegin() and rend(),
// see bug 1964967.
for (FactoryOp* existingOp = gFactoryOps->getLast(); existingOp;
existingOp = existingOp->getPrevious()) {
if (existingOp == &self) { if (existingOp == &self) {
foundThis = true; foundThis = true;
continue; continue;
@@ -15005,7 +15014,7 @@ void FactoryOp::CleanupMetadata() {
mBlocking.Clear(); mBlocking.Clear();
MOZ_ASSERT(gFactoryOps); MOZ_ASSERT(gFactoryOps);
gFactoryOps->RemoveElement(this); removeFrom(*gFactoryOps);
// We might get here even after QuotaManagerOpen failed, so we need to check // We might get here even after QuotaManagerOpen failed, so we need to check
// if we have a quota manager. // if we have a quota manager.