Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove diagnostic logging from CachedResourceLoader #20107

Conversation

Constellation
Copy link
Member

@Constellation Constellation commented Nov 7, 2023

c1ff317

Remove diagnostic logging from CachedResourceLoader
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

83a513c

Misc iOS, tvOS & watchOS macOS Linux Windows
✅ 🧪 style ✅ 🛠 ios ✅ 🛠 mac ✅ 🛠 wpe ✅ 🛠 wincairo
✅ 🧪 bindings ✅ 🛠 ios-sim ✅ 🛠 mac-AS-debug 🧪 wpe-wk2
✅ 🧪 webkitperl ✅ 🧪 ios-wk2 🧪 api-mac ✅ 🛠 gtk
✅ 🧪 ios-wk2-wpt ✅ 🧪 mac-wk1 🧪 gtk-wk2
✅ 🧪 api-ios ✅ 🧪 mac-wk2 ✅ 🧪 api-gtk
🛠 tv 🧪 mac-AS-debug-wk2
✅ 🛠 tv-sim
✅ 🛠 watch ⏳ 🛠 jsc-mips
✅ 🛠 🧪 unsafe-merge ✅ 🛠 watch-sim ⏳ 🧪 jsc-mips-tests

@Constellation Constellation self-assigned this Nov 7, 2023
@Constellation Constellation added the Page Loading For bugs in page loading, including handling of network callbacks. label Nov 7, 2023
Copy link
Contributor

@cdumez cdumez left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@webkit-ews-buildbot webkit-ews-buildbot added the merging-blocked Applied to prevent a change from being merged label Nov 7, 2023
@Constellation Constellation removed the merging-blocked Applied to prevent a change from being merged label Nov 7, 2023
@Constellation Constellation force-pushed the eng/Remove-diagnostic-logging-from-CachedResourceLoader branch from f2a50fb to 83a513c Compare November 7, 2023 18:17
@Constellation Constellation added the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 7, 2023
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
@webkit-commit-queue webkit-commit-queue force-pushed the eng/Remove-diagnostic-logging-from-CachedResourceLoader branch from 83a513c to c1ff317 Compare November 7, 2023 19:05
@webkit-commit-queue
Copy link
Collaborator

Committed 270338@main (c1ff317): https://commits.webkit.org/270338@main

Reviewed commits have been landed. Closing PR #20107 and removing active labels.

@webkit-commit-queue webkit-commit-queue merged commit c1ff317 into WebKit:main Nov 7, 2023
@webkit-commit-queue webkit-commit-queue removed the unsafe-merge-queue Applied to send a pull request to merge-queue, but skip building and testing label Nov 7, 2023
@Constellation Constellation deleted the eng/Remove-diagnostic-logging-from-CachedResourceLoader branch November 8, 2023 19:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Page Loading For bugs in page loading, including handling of network callbacks.
Projects
None yet
5 participants