Skip to content

Commit

Permalink
Remove diagnostic logging from CachedResourceLoader
Browse files Browse the repository at this point in the history 
https://bugs.webkit.org/show_bug.cgi?id=264340
rdar://118061900

Reviewed by Chris Dumez.

These loggings are no longer used. And it turned out that they are very
costly when we are just hitting memory-cached resource path. We are sampling
with 5%, which means we hit this every 20 image elements for example.
This patch drops it from CachedResourceLoader. We should consider removing
the other loggings too in the subsequent patch.

* Source/WebCore/loader/cache/CachedResourceLoader.cpp:
(WebCore::CachedResourceLoader::requestResource):
(WebCore::CachedResourceLoader::determineRevalidationPolicy const):
(WebCore::logMemoryCacheResourceRequest): Deleted.
(WebCore::logRevalidation): Deleted.
(WebCore::logResourceRevalidationDecision): Deleted.
* Source/WebCore/page/DiagnosticLoggingKeys.cpp:
(WebCore::DiagnosticLoggingKeys::noCacheKey): Deleted.
(WebCore::DiagnosticLoggingKeys::noStoreKey): Deleted.
(WebCore::DiagnosticLoggingKeys::notInMemoryCacheKey): Deleted.
(WebCore::DiagnosticLoggingKeys::isExpiredKey): Deleted.
(WebCore::DiagnosticLoggingKeys::inMemoryCacheKey): Deleted.
(WebCore::DiagnosticLoggingKeys::reloadKey): Deleted.
(WebCore::DiagnosticLoggingKeys::revalidatingKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonCredentialSettingsKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonErrorKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonMustRevalidateNoValidatorKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonNoStoreKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonRedirectChainKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonReloadKey): Deleted.
(WebCore::DiagnosticLoggingKeys::unusedReasonTypeMismatchKey): Deleted.
* Source/WebCore/page/DiagnosticLoggingKeys.h:

Canonical link: https://commits.webkit.org/270338@main
  • Loading branch information
@Constellation
Constellation committed Nov 7, 2023
1 parent 347e859 commit c1ff317
Show file tree
Hide file tree
Showing 3 changed files with 1 addition and 131 deletions.
54 changes: 1 addition & 53 deletions Source/WebCore/loader/cache/CachedResourceLoader.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -47,8 +47,6 @@
#include "CrossOriginAccessControl.h"
#include "CustomHeaderFields.h"
#include "DateComponents.h"
#include "DiagnosticLoggingClient.h"
#include "DiagnosticLoggingKeys.h"
#include "DocumentInlines.h"
#include "DocumentLoader.h"
#include "FrameLoader.h"
Expand Down Expand Up @@ -867,13 +865,6 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::updateCachedResourceW
return resourceHandle;
}

static inline void logMemoryCacheResourceRequest(LocalFrame* frame, const String& key, const String& description)
{
if (!frame || !frame->page())
return;
frame->page()->diagnosticLoggingClient().logDiagnosticMessage(key, description, ShouldSample::Yes);
}

void CachedResourceLoader::prepareFetch(CachedResource::Type type, CachedResourceRequest& request)
{
// Implementing step 1.1 to 1.6 of https://fetch.spec.whatwg.org/#concept-fetch.
Expand Down Expand Up @@ -1115,8 +1106,6 @@ ResourceErrorOr<CachedResourceHandle<CachedResource>> CachedResourceLoader::requ
if (resource && request.isLinkPreload() && !resource->isLinkPreload())
resource->setLinkPreload();

logMemoryCacheResourceRequest(&frame, DiagnosticLoggingKeys::memoryCacheUsageKey(), resource ? DiagnosticLoggingKeys::inMemoryCacheKey() : DiagnosticLoggingKeys::notInMemoryCacheKey());

auto& cookieJar = page.cookieJar();

RevalidationPolicy policy = determineRevalidationPolicy(type, request, resource.get(), forPreload, imageLoading);
Expand All @@ -1125,15 +1114,11 @@ ResourceErrorOr<CachedResourceHandle<CachedResource>> CachedResourceLoader::requ
memoryCache.remove(*resource);
FALLTHROUGH;
case Load:
if (resource) {
logMemoryCacheResourceRequest(&frame, DiagnosticLoggingKeys::memoryCacheEntryDecisionKey(), DiagnosticLoggingKeys::unusedKey());
if (resource)
memoryCache.remove(*resource);
}
resource = loadResource(type, page.sessionID(), WTFMove(request), cookieJar, page.settings());
break;
case Revalidate:
if (resource)
logMemoryCacheResourceRequest(&frame, DiagnosticLoggingKeys::memoryCacheEntryDecisionKey(), DiagnosticLoggingKeys::revalidatingKey());
resource = revalidateResource(WTFMove(request), *resource);
break;
case Use:
Expand Down Expand Up @@ -1166,7 +1151,6 @@ ResourceErrorOr<CachedResourceHandle<CachedResource>> CachedResourceLoader::requ
ResourceError error;
if (!shouldContinueAfterNotifyingLoadedFromMemoryCache(request, *resource, error))
return makeUnexpected(WTFMove(error));
logMemoryCacheResourceRequest(&frame, DiagnosticLoggingKeys::memoryCacheEntryDecisionKey(), DiagnosticLoggingKeys::usedKey());
loadTiming.markEndTime();

memoryCache.resourceAccessed(*resource);
Expand Down Expand Up @@ -1285,34 +1269,6 @@ CachedResourceHandle<CachedResource> CachedResourceLoader::loadResource(CachedRe
return resource;
}

static void logRevalidation(const String& reason, DiagnosticLoggingClient& logClient)
{
logClient.logDiagnosticMessage(DiagnosticLoggingKeys::cachedResourceRevalidationReasonKey(), reason, ShouldSample::Yes);
}

static void logResourceRevalidationDecision(CachedResource::RevalidationDecision reason, const LocalFrame* frame)
{
if (!frame || !frame->page())
return;
auto& logClient = frame->page()->diagnosticLoggingClient();
switch (reason) {
case CachedResource::RevalidationDecision::No:
break;
case CachedResource::RevalidationDecision::YesDueToExpired:
logRevalidation(DiagnosticLoggingKeys::isExpiredKey(), logClient);
break;
case CachedResource::RevalidationDecision::YesDueToNoStore:
logRevalidation(DiagnosticLoggingKeys::noStoreKey(), logClient);
break;
case CachedResource::RevalidationDecision::YesDueToNoCache:
logRevalidation(DiagnosticLoggingKeys::noCacheKey(), logClient);
break;
case CachedResource::RevalidationDecision::YesDueToCachePolicy:
logRevalidation(DiagnosticLoggingKeys::reloadKey(), logClient);
break;
}
}

#if ENABLE(SERVICE_WORKER)
static inline bool mustReloadFromServiceWorkerOptions(const ResourceLoaderOptions& options, const ResourceLoaderOptions& cachedOptions)
{
Expand Down Expand Up @@ -1354,7 +1310,6 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
// If the same URL has been loaded as a different type, we need to reload.
if (existingResource->type() != type) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading due to type mismatch.");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonTypeMismatchKey());
return Reload;
}

Expand Down Expand Up @@ -1409,7 +1364,6 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
bool cachePolicyIsHistoryBuffer = cachePolicy == CachePolicy::HistoryBuffer;
if (!existingResource->redirectChainAllowsReuse(cachePolicyIsHistoryBuffer ? ReuseExpiredRedirection : DoNotReuseExpiredRedirection)) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading due to not cached or expired redirections.");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonRedirectChainKey());
return Reload;
}

Expand All @@ -1424,7 +1378,6 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
// Don't reuse resources with Cache-control: no-store.
if (existingResource->response().cacheControlContainsNoStore()) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading due to Cache-control: no-store.");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonNoStoreKey());
return Reload;
}

Expand All @@ -1436,7 +1389,6 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
// client's requests are made without CORS and some with.
if (existingResource->resourceRequest().allowCookies() != request.allowCookies() || existingResource->options().credentials != cachedResourceRequest.options().credentials) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading due to difference in credentials settings.");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonCredentialSettingsKey());
return Reload;
}

Expand All @@ -1447,14 +1399,12 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
// CachePolicy::Reload always reloads
if (cachePolicy == CachePolicy::Reload) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading due to CachePolicyReload.");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonReloadKey());
return Reload;
}

// We'll try to reload the resource if it failed last time.
if (existingResource->errorOccurred()) {
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicye reloading due to resource being in the error state");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonErrorKey());
return Reload;
}

Expand All @@ -1470,7 +1420,6 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida
}

auto revalidationDecision = existingResource->makeRevalidationDecision(cachePolicy);
logResourceRevalidationDecision(revalidationDecision, frame());

// Check if the cache headers requires us to revalidate (cache expiration for example).
if (revalidationDecision != CachedResource::RevalidationDecision::No) {
Expand All @@ -1486,7 +1435,6 @@ CachedResourceLoader::RevalidationPolicy CachedResourceLoader::determineRevalida

// No, must reload.
LOG(ResourceLoading, "CachedResourceLoader::determineRevalidationPolicy reloading due to missing cache validators.");
logMemoryCacheResourceRequest(frame(), DiagnosticLoggingKeys::inMemoryCacheKey(), DiagnosticLoggingKeys::unusedReasonMustRevalidateNoValidatorKey());
return Reload;
}

Expand Down
65 changes: 0 additions & 65 deletions Source/WebCore/page/DiagnosticLoggingKeys.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -168,26 +168,11 @@ String DiagnosticLoggingKeys::noKey()
return "no"_s;
}

String DiagnosticLoggingKeys::noCacheKey()
{
return "noCache"_s;
}

String DiagnosticLoggingKeys::noStoreKey()
{
return "noStore"_s;
}

String DiagnosticLoggingKeys::nonVisibleStateKey()
{
return "nonVisibleState"_s;
}

String DiagnosticLoggingKeys::notInMemoryCacheKey()
{
return "notInMemoryCache"_s;
}

String DiagnosticLoggingKeys::backForwardCacheKey()
{
return "backForwardCache"_s;
Expand Down Expand Up @@ -228,11 +213,6 @@ String DiagnosticLoggingKeys::isErrorPageKey()
return "isErrorPage"_s;
}

String DiagnosticLoggingKeys::isExpiredKey()
{
return "isExpired"_s;
}

String DiagnosticLoggingKeys::isReloadIgnoringCacheDataKey()
{
return "isReloadIgnoringCacheData"_s;
Expand All @@ -253,11 +233,6 @@ String DiagnosticLoggingKeys::imageKey()
return "image"_s;
}

String DiagnosticLoggingKeys::inMemoryCacheKey()
{
return "inMemoryCache"_s;
}

String DiagnosticLoggingKeys::inactiveKey()
{
return "inactive"_s;
Expand Down Expand Up @@ -490,11 +465,6 @@ String DiagnosticLoggingKeys::retrievalKey()
return "retrieval"_s;
}

String DiagnosticLoggingKeys::revalidatingKey()
{
return "revalidating"_s;
}

String DiagnosticLoggingKeys::reloadFromOriginKey()
{
return "reloadFromOrigin"_s;
Expand Down Expand Up @@ -665,41 +635,6 @@ String DiagnosticLoggingKeys::unusedKey()
return "unused"_s;
}

String DiagnosticLoggingKeys::unusedReasonCredentialSettingsKey()
{
return "unused.reason.credentialSettings"_s;
}

String DiagnosticLoggingKeys::unusedReasonErrorKey()
{
return "unused.reason.error"_s;
}

String DiagnosticLoggingKeys::unusedReasonMustRevalidateNoValidatorKey()
{
return "unused.reason.mustRevalidateNoValidator"_s;
}

String DiagnosticLoggingKeys::unusedReasonNoStoreKey()
{
return "unused.reason.noStore"_s;
}

String DiagnosticLoggingKeys::unusedReasonRedirectChainKey()
{
return "unused.reason.redirectChain"_s;
}

String DiagnosticLoggingKeys::unusedReasonReloadKey()
{
return "unused.reason.reload"_s;
}

String DiagnosticLoggingKeys::unusedReasonTypeMismatchKey()
{
return "unused.reason.typeMismatch"_s;
}

String DiagnosticLoggingKeys::usedKey()
{
return "used"_s;
Expand Down
13 changes: 0 additions & 13 deletions Source/WebCore/page/DiagnosticLoggingKeys.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -77,15 +77,13 @@ class DiagnosticLoggingKeys {
static String fontKey();
static String httpsNoStoreKey();
static String imageKey();
static String inMemoryCacheKey();
WEBCORE_EXPORT static String inactiveKey();
WEBCORE_EXPORT static String internalErrorKey();
WEBCORE_EXPORT static String invalidSessionIDKey();
WEBCORE_EXPORT static String isAttachmentKey();
WEBCORE_EXPORT static String isConditionalRequestKey();
static String isDisabledKey();
static String isErrorPageKey();
static String isExpiredKey();
WEBCORE_EXPORT static String isReloadIgnoringCacheDataKey();
static String loadingKey();
static String isLoadingKey();
Expand All @@ -105,14 +103,11 @@ class DiagnosticLoggingKeys {
WEBCORE_EXPORT static String networkProcessCrashedKey();
WEBCORE_EXPORT static String neverSeenBeforeKey();
static String noKey();
static String noCacheKey();
static String noCurrentHistoryItemKey();
static String noDocumentLoaderKey();
WEBCORE_EXPORT static String noLongerInCacheKey();
static String noStoreKey();
WEBCORE_EXPORT static String nonVisibleStateKey();
WEBCORE_EXPORT static String notHTTPFamilyKey();
static String notInMemoryCacheKey();
WEBCORE_EXPORT static String occurredKey();
WEBCORE_EXPORT static String otherKey();
static String backForwardCacheKey();
Expand Down Expand Up @@ -142,7 +137,6 @@ class DiagnosticLoggingKeys {
static String resourceResponseSourceKey();
WEBCORE_EXPORT static String retrievalKey();
WEBCORE_EXPORT static String retrievalRequestKey();
WEBCORE_EXPORT static String revalidatingKey();
static String sameLoadKey();
static String scriptKey();
static String serviceWorkerKey();
Expand All @@ -166,13 +160,6 @@ class DiagnosticLoggingKeys {
WEBCORE_EXPORT static String unsupportedHTTPMethodKey();
static String unsuspendableDOMObjectKey();
WEBCORE_EXPORT static String unusedKey();
static String unusedReasonCredentialSettingsKey();
static String unusedReasonErrorKey();
static String unusedReasonMustRevalidateNoValidatorKey();
static String unusedReasonNoStoreKey();
static String unusedReasonRedirectChainKey();
static String unusedReasonReloadKey();
static String unusedReasonTypeMismatchKey();
static String usedKey();
WEBCORE_EXPORT static String userZoomActionKey();
WEBCORE_EXPORT static String varyingHeaderMismatchKey();
Expand Down

0 comments on commit c1ff317

Please sign in to comment.