From 8c46126007e39319e1e8631468a54d078f315361 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Thu, 22 Jul 2021 23:26:36 -0400 Subject: [PATCH 1/2] Suppress vault callbacks when loading or unloading VNodeMgrs. On MOULa, there have been complaints about linking to some Neighborhoods and activating some players taking a very long time. One of these Neighborhoods in particular is the "DRC(67) Bevin". When I attempted to link to this Age, I found the link took approximately 85 seconds each time. On profiling, I discovered that for every node downloaded during the initialization phase, we were calling into Python at least once. Suppressing vault callbacks during times when they are obviously going to storm and be useless decreases the link time to 7 seconds. --- .../PubUtilLib/plVault/plVaultClientApi.cpp | 85 +++++++++++++------ .../PubUtilLib/plVault/plVaultClientApi.h | 12 +++ 2 files changed, 73 insertions(+), 24 deletions(-) diff --git a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp index bbb4e014..98b4eb03 100644 --- a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp +++ b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp @@ -181,7 +181,28 @@ struct VaultDownloadTrans { unsigned vaultId; ENetError result; - VaultDownloadTrans (); + VaultDownloadTrans() + : callback(), cbParam(), progressCallback(), cbProgressParam(), + nodeCount(), nodesLeft(), vaultId(), result(kNetSuccess) + { + VaultSuppressCallbacks(); + } + + VaultDownloadTrans(const wchar_t* _tag, FVaultDownloadCallback _callback, + void* _cbParam, FVaultProgressCallback _progressCallback, + void* _cbProgressParam, unsigned _vaultId) + : callback(_callback), cbParam(_cbParam), progressCallback(_progressCallback), + cbProgressParam(_cbProgressParam), nodeCount(), nodesLeft(), + vaultId(_vaultId), result(kNetSuccess) + { + StrCopy(tag, _tag, MAX_PATH); + VaultSuppressCallbacks(); + } + + ~VaultDownloadTrans() + { + VaultEnableCallbacks(); + } static void VaultNodeFetched ( ENetError result, @@ -262,6 +283,8 @@ static HASHTABLEDECL( static bool s_processPlayerInbox = false; +static long s_suppressCallbacks = 0; + /***************************************************************************** * * Local functions @@ -303,8 +326,10 @@ static void VaultNodeAddedDownloadCallback(ENetError result, void * param) { VaultProcessUnvisitNote(childLink->node); } - for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) - cb->cb->AddedChildNode(parentLink->node, childLink->node); + if (s_suppressCallbacks == 0) { + for (IVaultCallback* cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) + cb->cb->AddedChildNode(parentLink->node, childLink->node); + } } } @@ -377,9 +402,11 @@ static void BuildNodeTree ( parentNode->state->children.Add(childLink); if (notifyNow || childNode->nodeType != 0) { - // We made a new link, so make the callbacks - for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) - cb->cb->AddedChildNode(parentNode, childNode); + if (s_suppressCallbacks == 0) { + // We made a new link, so make the callbacks + for (IVaultCallback* cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) + cb->cb->AddedChildNode(parentNode, childNode); + } } else { INotifyAfterDownload* notify = NEWZERO(INotifyAfterDownload)(parentNode->nodeId, childNode->nodeId); @@ -543,6 +570,10 @@ static void ChangedVaultNodeFetched ( RelVaultNodeLink* savedLink = s_nodes.Find(node->nodeId); + // Yeah, we are purposefully allowing this global callback to go out, + // even if callback suppression has been enabled. The intent behind + // that is to suppress spurious callbacks, but node changes are + // probably not spurious. if (savedLink) { for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) cb->cb->ChangedNode(savedLink->node); @@ -638,7 +669,7 @@ static void VaultNodeAdded ( RelVaultNodeLink* parentLink = s_nodes.Find(parentId); RelVaultNodeLink* childLink = s_nodes.Find(childId); - if (childLink->node->nodeType != 0) { + if (childLink->node->nodeType != 0 && s_suppressCallbacks == 0) { for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) cb->cb->AddedChildNode(parentLink->node, childLink->node); } @@ -660,7 +691,7 @@ static void VaultNodeRemoved ( if (!childLink) break; - if (parentLink->node->IsParentOf(childId, 1)) { + if (parentLink->node->IsParentOf(childId, 1) && s_suppressCallbacks == 0) { // We have the relationship, so make the callbacks for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) cb->cb->RemovingChildNode(parentLink->node, childLink->node); @@ -831,11 +862,6 @@ void VaultFindNodeTrans::VaultNodeFound ( * ***/ -//============================================================================ -VaultDownloadTrans::VaultDownloadTrans () { - ASSERT(!nodeCount); // must be alloced with -} - //============================================================================ void VaultDownloadTrans::VaultNodeFetched ( ENetError result, @@ -1047,8 +1073,10 @@ void IRelVaultNode::UnlinkFromRelatives () { next = parents.Next(link); // We have the relationship, so make the callbacks - for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) - cb->cb->RemovingChildNode(link->node, this->node); + if (s_suppressCallbacks == 0) { + for (IVaultCallback* cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) + cb->cb->RemovingChildNode(link->node, this->node); + } link->node->state->Unlink(node); } @@ -1478,6 +1506,16 @@ void VaultUnregisterCallback (VaultCallback * cb) { cb->internal = nil; } +//============================================================================ +void VaultSuppressCallbacks() { + AtomicAdd(&s_suppressCallbacks, 1); +} + +//============================================================================ +void VaultEnableCallbacks() { + long prevValue = AtomicAdd(&s_suppressCallbacks, -1); + hsAssert(prevValue > 0, "Hmm... A negative vault callback suppression count?"); +} /***************************************************************************** * @@ -1772,8 +1810,10 @@ void VaultRemoveChildNode ( if (parentLink->node->IsParentOf(childId, 1)) { // We have the relationship, so make the callbacks - for (IVaultCallback * cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) - cb->cb->RemovingChildNode(parentLink->node, childLink->node); + if (s_suppressCallbacks == 0) { + for (IVaultCallback* cb = s_callbacks.Head(); cb; cb = s_callbacks.Next(cb)) + cb->cb->RemovingChildNode(parentLink->node, childLink->node); + } } parentLink->node->state->Unlink(childLink->node); @@ -4945,13 +4985,8 @@ void VaultDownload ( FVaultProgressCallback progressCallback, void * cbProgressParam ) { - VaultDownloadTrans * trans = NEWZERO(VaultDownloadTrans); - StrCopy(trans->tag, tag, arrsize(trans->tag)); - trans->callback = callback; - trans->cbParam = cbParam; - trans->progressCallback = progressCallback; - trans->cbProgressParam = cbProgressParam; - trans->vaultId = vaultId; + VaultDownloadTrans * trans = new VaultDownloadTrans(tag, callback, cbParam, + progressCallback, cbProgressParam, vaultId); NetCliAuthVaultFetchNodeRefs( vaultId, @@ -5001,6 +5036,8 @@ void VaultDownloadAndWait ( //============================================================================ void VaultCull (unsigned vaultId) { + VaultCallbackSuppressor suppress; + // Remove the node from the global table if (RelVaultNodeLink * link = s_nodes.Find(vaultId)) { LogMsg(kLogDebug, L"Vault: Culling node %u", link->node->nodeId); diff --git a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h index 39bd0aa0..3ecd968b 100644 --- a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h +++ b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h @@ -82,6 +82,18 @@ struct VaultCallback { void VaultRegisterCallback (VaultCallback * cb); void VaultUnregisterCallback (VaultCallback * cb); +void VaultSuppressCallbacks(); +void VaultEnableCallbacks(); + +class VaultCallbackSuppressor +{ + VaultCallbackSuppressor(const VaultCallbackSuppressor&) { } + VaultCallbackSuppressor(VaultCallbackSuppressor&&) { } + +public: + VaultCallbackSuppressor() { VaultSuppressCallbacks(); } + ~VaultCallbackSuppressor() { VaultEnableCallbacks(); } +}; /***************************************************************************** * From d007fac5364823fbdfdde1081ea3c30fde5e6810 Mon Sep 17 00:00:00 2001 From: Adam Johnson Date: Wed, 28 Jul 2021 22:27:26 -0400 Subject: [PATCH 2/2] Limit the scope of callback suppression. Per testing on Minkata, suppressing callbacks during all vault downloads has a deleterious effect on imagers. While my high level assumption is correct, vault downloads can encompass situations where we want notifications (eg re-downloading imager inbox folders, new age info vault fragments). Whoops! --- .../plNetClient/plNetCliAgeJoiner.cpp | 2 +- .../plNetClientComm/plNetClientComm.cpp | 4 +- .../PubUtilLib/plVault/plVaultClientApi.cpp | 52 ++++++++++++++++--- .../PubUtilLib/plVault/plVaultClientApi.h | 8 +++ 4 files changed, 55 insertions(+), 11 deletions(-) diff --git a/Sources/Plasma/PubUtilLib/plNetClient/plNetCliAgeJoiner.cpp b/Sources/Plasma/PubUtilLib/plNetClient/plNetCliAgeJoiner.cpp index d934ed81..6581052d 100644 --- a/Sources/Plasma/PubUtilLib/plNetClient/plNetCliAgeJoiner.cpp +++ b/Sources/Plasma/PubUtilLib/plNetClient/plNetCliAgeJoiner.cpp @@ -385,7 +385,7 @@ bool plNCAgeJoiner::MsgReceive (plMessage * msg) { } else if (unsigned ageVaultId = NetCommGetAge()->ageVaultId) { // Download the age vault - VaultDownload( + VaultDownloadNoCallbacks( L"AgeJoin", ageVaultId, AgeVaultDownloadCallback, diff --git a/Sources/Plasma/PubUtilLib/plNetClientComm/plNetClientComm.cpp b/Sources/Plasma/PubUtilLib/plNetClientComm/plNetClientComm.cpp index 3ebb2dc0..342897b7 100644 --- a/Sources/Plasma/PubUtilLib/plNetClientComm/plNetClientComm.cpp +++ b/Sources/Plasma/PubUtilLib/plNetClientComm/plNetClientComm.cpp @@ -318,7 +318,7 @@ static void INetCliAuthSetPlayerRequestCallback ( else { s_needAvatarLoad = true; - VaultDownload( + VaultDownloadNoCallbacks( L"SetActivePlayer", s_player->playerInt, PlayerInitCallback, @@ -374,7 +374,7 @@ static void INetCliAuthLoginSetPlayerRequestCallback ( msg->Send(); } else { - VaultDownload( + VaultDownloadNoCallbacks( L"SetActivePlayer", s_player->playerInt, LoginPlayerInitCallback, diff --git a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp index 98b4eb03..66a91d4b 100644 --- a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp +++ b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.cpp @@ -185,7 +185,6 @@ struct VaultDownloadTrans { : callback(), cbParam(), progressCallback(), cbProgressParam(), nodeCount(), nodesLeft(), vaultId(), result(kNetSuccess) { - VaultSuppressCallbacks(); } VaultDownloadTrans(const wchar_t* _tag, FVaultDownloadCallback _callback, @@ -195,15 +194,11 @@ struct VaultDownloadTrans { cbProgressParam(_cbProgressParam), nodeCount(), nodesLeft(), vaultId(_vaultId), result(kNetSuccess) { - StrCopy(tag, _tag, MAX_PATH); - VaultSuppressCallbacks(); + StrCopy(tag, _tag, arrsize(tag)); } - ~VaultDownloadTrans() - { - VaultEnableCallbacks(); - } - + virtual ~VaultDownloadTrans() { } + static void VaultNodeFetched ( ENetError result, void * param, @@ -217,6 +212,28 @@ struct VaultDownloadTrans { ); }; + +struct VaultDownloadNoCallbacksTrans : VaultDownloadTrans { + VaultDownloadNoCallbacksTrans() + : VaultDownloadTrans() + { + VaultSuppressCallbacks(); + } + + VaultDownloadNoCallbacksTrans(const wchar_t* _tag, FVaultDownloadCallback _callback, + void* _cbParam, FVaultProgressCallback _progressCallback, + void* _cbProgressParam, unsigned _vaultId) + : VaultDownloadTrans(_tag, _callback, _cbParam, _progressCallback, _cbProgressParam, _vaultId) + { + VaultSuppressCallbacks(); + } + + ~VaultDownloadNoCallbacksTrans() + { + VaultEnableCallbacks(); + } +}; + struct VaultAgeInitTrans { FVaultInitAgeCallback callback; void * cbState; @@ -4995,6 +5012,25 @@ void VaultDownload ( ); } +//============================================================================ +void VaultDownloadNoCallbacks ( + const wchar tag[], + unsigned vaultId, + FVaultDownloadCallback callback, + void* cbParam, + FVaultProgressCallback progressCallback, + void* cbProgressParam +) { + VaultDownloadNoCallbacksTrans* trans = new VaultDownloadNoCallbacksTrans(tag, callback, + cbParam, progressCallback, cbProgressParam, vaultId); + + NetCliAuthVaultFetchNodeRefs( + vaultId, + VaultDownloadTrans::VaultNodeRefsFetched, + trans + ); +} + //============================================================================ struct _DownloadVaultParam { ENetError result; diff --git a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h index 3ecd968b..ba38536f 100644 --- a/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h +++ b/Sources/Plasma/PubUtilLib/plVault/plVaultClientApi.h @@ -488,6 +488,14 @@ void VaultDownload ( FVaultProgressCallback progressCallback, void * cbProgressParam ); +void VaultDownloadNoCallbacks( + const wchar tag[], + unsigned vaultId, + FVaultDownloadCallback callback, + void* cbParam, + FVaultProgressCallback progressCallback, + void* cbProgressParam +); void VaultDownloadAndWait ( const wchar tag[], unsigned vaultId,