While profiling the load of nytimes.com, I noticed that the site is accessing ~250 times document.cookie, just during page load. Accessing document.cookie is currently slow because we: - Call WebPlatformStrategies::cookiesForDOM() virtual function - Send a sync IPC message to the Network process to retrieve the cookies * The Network process gets the list of cookies from CFNetwork then serializes the result to send it back to the WebProcess - We unserialize the cookies into an NSList of cookies - We filter-out the cookies that are 'httpOnly' and construct a new NSList of cookies - We create a WTF String out of the cookies NSList In the case of nytimes.com, it turns out that up to 100 calls to document.cookie() are made in the same event loop iteration. A proposal would be to cache / freeze the cookies until we return to the event loop so that consecutive calls to document.cookie() can be fast. Doing so seems to be sufficient to achieve a ~76% cache hit for nytimes.com.
Created attachment 238869 [details] WIP Patch I still likely need to invalidate the cache in case of sync XHRs. This case doesn't seem to be covered by existing layout tests so I'll need to add coverage.
Comment on attachment 238869 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238869&action=review > Source/WebCore/dom/Document.h:1709 > + String m_cookiesCache; It may be worth being more clear that these are DOM cookies (i.e. httponly cookies are not there).
Attachment 238869 [details] did not pass style-queue: ERROR: Source/WebCore/ChangeLog:14: Need whitespace between colon and description [changelog/filechangedescriptionwhitespace] [5] Total errors found: 1 in 3 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment on attachment 238869 [details] WIP Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238869&action=review >> Source/WebCore/dom/Document.h:1709 >> + String m_cookiesCache; > > It may be worth being more clear that these are DOM cookies (i.e. httponly cookies are not there). Good point, I'll add "DOM" in the names.
Created attachment 238887 [details] WIP Patch
Created attachment 238891 [details] Patch
Comment on attachment 238891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238891&action=review > Source/WebCore/dom/Document.cpp:3860 > + if (!isDOMCookiesCacheValid()) > + setDOMCookiesCache(cookies(this, cookieURL)); > + > + return domCookiesCache(); I think that it's "cookie cache", not "cookies cache". > Source/WebCore/dom/Document.h:1375 > + void didDOMCookiesCacheExpiryTimerFired(Timer<Document>&); Syntax is wrong here "did fired" doesn't parse. Maybe "domCookieCacheExpiryTimerFired"? > Source/WebCore/dom/Document.h:1376 > + void didLoadXHRSynchronously() override final; Can (or should) we catch it more generally, e.g. invalidate the cache when exiting any nested run loop? Another case I have in mind is showModalDialog. Also XSLT stylesheet loading. > Source/WebCore/dom/ScriptExecutionContext.h:114 > + // XHR. > + virtual void didLoadXHRSynchronously(); This comment doesn't seem helpful.
Comment on attachment 238891 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238891&action=review >> Source/WebCore/dom/Document.h:1376 >> + void didLoadXHRSynchronously() override final; > > Can (or should) we catch it more generally, e.g. invalidate the cache when exiting any nested run loop? Another case I have in mind is showModalDialog. Also XSLT stylesheet loading. I am not sure how to do this but I will try
Created attachment 238896 [details] Patch
(In reply to comment #8) > (From update of attachment 238891 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238891&action=review > > >> Source/WebCore/dom/Document.h:1376 > >> + void didLoadXHRSynchronously() override final; > > > > Can (or should) we catch it more generally, e.g. invalidate the cache when exiting any nested run loop? Another case I have in mind is showModalDialog. Also XSLT stylesheet loading. > > I am not sure how to do this but I will try Alexey, how about invalidating the Document's cookie cache when ThreadableLoader::loadResourceSynchronously() is called? This will cover the sync XHR case and more cases where network resources are loaded synchronously. This will hopefully be safer.
Good idea!
Created attachment 238939 [details] Patch
Comment on attachment 238939 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238939&action=review > Source/WebCore/dom/Document.cpp:6197 > + // The cookies cache is valid until we go back to the event loop. "cookie cache" > Source/WebCore/dom/Document.cpp:6198 > + m_cookieCacheExpiryTimer.startOneShot(0); It would be nice not to rely on this race to expire the cache. It's possible for a network resource and a zero-delay JavaScript timer to run in the next runloop iteration before this invalidation event. I wonder if there's a low-level networking API we can use to invalidate the cache instead. For example, is the document notified when a subresource loads?
(In reply to comment #13) > (From update of attachment 238939 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=238939&action=review > > > Source/WebCore/dom/Document.cpp:6197 > > + // The cookies cache is valid until we go back to the event loop. > > "cookie cache" > > > Source/WebCore/dom/Document.cpp:6198 > > + m_cookieCacheExpiryTimer.startOneShot(0); > > It would be nice not to rely on this race to expire the cache. It's possible for a network resource and a zero-delay JavaScript timer to run in the next runloop iteration before this invalidation event. I wonder if there's a low-level networking API we can use to invalidate the cache instead. For example, is the document notified when a subresource loads? Knowing about sub-resources loaded for the current WebProcess sounds uncomplicated. The issue is knowing about the sub-resources loaded for other WebProcesses as they can alter the cookies as well.
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 238939 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=238939&action=review > > > > > Source/WebCore/dom/Document.cpp:6197 > > > + // The cookies cache is valid until we go back to the event loop. > > > > "cookie cache" > > > > > Source/WebCore/dom/Document.cpp:6198 > > > + m_cookieCacheExpiryTimer.startOneShot(0); > > > > It would be nice not to rely on this race to expire the cache. It's possible for a network resource and a zero-delay JavaScript timer to run in the next runloop iteration before this invalidation event. I wonder if there's a low-level networking API we can use to invalidate the cache instead. For example, is the document notified when a subresource loads? > > Knowing about sub-resources loaded for the current WebProcess sounds uncomplicated. The issue is knowing about the sub-resources loaded for other WebProcesses as they can alter the cookies as well. Ideally, we would just get a notification when cookies are changed. Invalidating the cookie cache for all resources loads would be less optimal. However, I was trying to find an acceptable short-term solution to caching Document.cookie.
What is the use case for caring about cookies that changed from another tab? I'm not sure if it's even correct to respect those changes before the next run loop iteration.
(In reply to comment #16) > What is the use case for caring about cookies that changed from another tab? I'm not sure if it's even correct to respect those changes before the next run loop iteration. Oh OK, then you would want to keep both right? 1. Invalidate in a setTimeout(0) (as in my patch) + 2. Invalidate if a sub resource load finishes for the current WebProcess
I liked the approach with loadResourceSynchronously more, even though there is probably no difference in practice. Not everything loaded by a page is a subresource - notably, pings are not. So, resource loading feels like the wrong level to implement this at.
(In reply to comment #18) > I liked the approach with loadResourceSynchronously more, even though there is probably no difference in practice. Not everything loaded by a page is a subresource - notably, pings are not. So, resource loading feels like the wrong level to implement this at. This is what my last patch iteration does (invalidate cache in loadResourceSynchronously).
Created attachment 238951 [details] Patch
Created attachment 238977 [details] Patch
Comment on attachment 238977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238977&action=review > Source/WebCore/dom/Document.cpp:3858 > + setDOMCookieCache(cookies(this, cookieURL)); setDOMCookieCache sounds like it's taking some kind of CookieCache object, which it is not. Maybe setCachedDOMCookies would be less confusing? > Source/WebCore/dom/Document.cpp:3860 > + return domCookieCache(); Ditto. cachedDOMCookies? > Source/WebCore/dom/Document.cpp:3988 > + m_cookieURL = url; > + invalidateDOMCookieCache(); I'm wondering if we ever call this function with the same value repeatedly. It may not matter in practice, we probably don't do that between document.cookie.calls too often, if ever. > Source/WebCore/dom/Document.cpp:6197 > + // The cookie cache is valid until we go back to the event loop. Should probably say "at most", as sync loading can also invalidate it. > LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html:18 > +<script src='resources/cookies-test-post.js'></script> I'm somewhat concerned that this may not clean up the cookies before the next test. Could you please double-check that?
Comment on attachment 238977 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=238977&action=review >> Source/WebCore/dom/Document.cpp:3858 >> + setDOMCookieCache(cookies(this, cookieURL)); > > setDOMCookieCache sounds like it's taking some kind of CookieCache object, which it is not. Maybe setCachedDOMCookies would be less confusing? Right, done. >> Source/WebCore/dom/Document.cpp:3860 >> + return domCookieCache(); > > Ditto. cachedDOMCookies? Done. >> Source/WebCore/dom/Document.cpp:3988 >> + invalidateDOMCookieCache(); > > I'm wondering if we ever call this function with the same value repeatedly. It may not matter in practice, we probably don't do that between document.cookie.calls too often, if ever. I verified and we definitely call setCookieURL() with the same URL repeatedly so I added a check. Cache hit is up from 76% to 87% on nytimes.com load now. >> Source/WebCore/dom/Document.cpp:6197 >> + // The cookie cache is valid until we go back to the event loop. > > Should probably say "at most", as sync loading can also invalidate it. Done. >> LayoutTests/http/tests/cookies/sync-xhr-set-cookie-invalidates-cache.html:18 >> +<script src='resources/cookies-test-post.js'></script> > > I'm somewhat concerned that this may not clean up the cookies before the next test. Could you please double-check that? cookies-test-post.js seems to be clearing the cookies at the end: // Make sure that we do not leak any cookies. clearCookies();
Created attachment 239059 [details] Patch
Comment on attachment 239059 [details] Patch Actually, my new test causes http/tests/misc/empty-cookie.html to be flaky. I will investigate and fix the test.
Created attachment 239063 [details] Patch
Ok, I updated my layout test to fix the flakiness.
Comment on attachment 239063 [details] Patch Clearing flags on attachment: 239063 Committed r174190: <http://trac.webkit.org/changeset/174190>
All reviewed patches have been landed. Closing bug.