Bug 137869 - [Mac] Optimize cookiesForDOM() by filtering and serializing cookies in a single pass
Summary: [Mac] Optimize cookiesForDOM() by filtering and serializing cookies in a sing...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: DOM (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac Unspecified
: P2 Enhancement
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2014-10-19 18:33 PDT by Chris Dumez
Modified: 2014-10-24 12:42 PDT (History)
1 user (show)

See Also:


Attachments
Patch (5.15 KB, patch)
2014-10-19 18:40 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.09 KB, patch)
2014-10-19 19:46 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (5.04 KB, patch)
2014-10-19 20:55 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2014-10-19 18:33:59 PDT
Optimize cookiesForDOM() by filtering and serializing in 1 pass instead of 2.

Currently, when accessing document.cookie, we end up doing the following:
1. Call wkHTTPCookiesForURL() to get an NSArray of NSHTTPCookies.
2. Call filterCookies() to filter out cookies that are httpOnly or with an empty name, thus allocating a new NSMutableArray.
3. Call NSHTTPCookie's requestHeaderFieldsWithCookies() to serialize the cookies
4. Construct a WTF::String from the NSString*

There are several inefficiencies here:
1. We need to pre-filter the cookies and allocate a new NSMutableArray before calling requestHeaderFieldsWithCookies()
2. requestHeaderFieldsWithCookies() does more things that we actually need. It constructs a Dictionary of header fields, of which we query the "Cookie" field, even though we merely want a ';'-separated string representation of the cookies in "key=value" form.

My proposal is to take care of the string serialization ourselves using a StringBuilder as it is trivial to do. This also allows us to filter out the httpOnly/invalid cookies as we do the serialization instead of having a first pass for this.

When scrolling the http://www.apple.com/iphone/ entire page down, then up, I see that we spend 13.1% of the NetworkProcess time in WebCore::cookiesForDOM() (~96ms):
Running Time	Self		Symbol Name
96.0ms   13.1%	1.0	 	WebCore::cookiesForDOM(WebCore::NetworkStorageSession const&, WebCore::URL const&, WebCore::URL const&)
51.0ms    7.0%	0.0	 	 -[NSHTTPCookieStorage cookiesForURL:]
26.0ms    3.5%	0.0	 	 +[NSHTTPCookie requestHeaderFieldsWithCookies:]
8.0ms    1.0%	0.0	 	 WebCore::filterCookies(NSArray*)
6.0ms    0.8%	0.0	 	 WebCore::URL::operator NSURL*() const
1.0ms    0.1%	1.0	 	 objc_msgSend
1.0ms    0.1%	0.0	 	 CFRelease
1.0ms    0.1%	1.0	 	 WKHTTPCookiesForURL
1.0ms    0.1%	0.0	 	 WTF::String::String(NSString*)

As you can see, 4.5% of the time is spent in requestHeaderFieldsWithCookies + filterCookies. By doing this in 1 pass, as suggested, we can decrease the time spent in WebCore::cookiesForDOM() by almost 30% (~96ms -> ~74ms):
Running Time	Self		Symbol Name
74.0ms   11.4%	1.0	 	WebCore::cookiesForDOM(WebCore::NetworkStorageSession const&, WebCore::URL const&, WebCore::URL const&)
50.0ms    7.7%	0.0	 	 -[NSHTTPCookieStorage cookiesForURL:]
5.0ms    0.7%	0.0	 	 WebCore::URL::operator NSURL*() const
5.0ms    0.7%	0.0	 	 -[NSHTTPCookie name]
4.0ms    0.6%	0.0	 	 -[NSHTTPCookie value]
2.0ms    0.3%	0.0	 	 bmalloc::Deallocator::deallocateSlowCase(void*)
2.0ms    0.3%	2.0	 	 WTF::StringBuilder::append(WTF::String const&)
2.0ms    0.3%	0.0	 	 WTF::String::String(NSString*)
1.0ms    0.1%	0.0	 	 <Unknown Address>
1.0ms    0.1%	1.0	 	 WTF::StringBuilder::append(unsigned char const*, unsigned int)
1.0ms    0.1%	1.0	 	 objc_msgSend
Comment 1 Chris Dumez 2014-10-19 18:40:46 PDT
Created attachment 240091 [details]
Patch
Comment 2 Chris Dumez 2014-10-19 19:46:00 PDT
Created attachment 240092 [details]
Patch
Comment 3 Darin Adler 2014-10-19 20:18:42 PDT
Comment on attachment 240092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240092&action=review

> Source/WebCore/platform/network/mac/CookieJarMac.mm:88
> +        cookiesBuilder.append(String([cookie name]));

If performance is enough of an issue here, we should consider coming up with a refinement to StringBuilder that doesn’t require allocating a new String for each NSString.

> Source/WebCore/platform/network/mac/CookieJarMac.mm:92
> +        if (cookie != lastCookie)
> +            cookiesBuilder.appendLiteral("; ");

I think a cleaner way to write this is to put at the beginning of the loop:

    if (!cookiesBuilder.isEmpty())
        cookiesBuilder.appendLiteral("; ");

Then we don’t need lastCookie.
Comment 4 Chris Dumez 2014-10-19 20:26:37 PDT
Comment on attachment 240092 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=240092&action=review

>> Source/WebCore/platform/network/mac/CookieJarMac.mm:88
>> +        cookiesBuilder.append(String([cookie name]));
> 
> If performance is enough of an issue here, we should consider coming up with a refinement to StringBuilder that doesn’t require allocating a new String for each NSString.

Yes, this is a bit unfortunate. However, at the moment, according to the profile (see bottom of the bug description), the impact seems fairly minimal:
2.0ms    0.3%	0.0	 	 WTF::String::String(NSString*)

>> Source/WebCore/platform/network/mac/CookieJarMac.mm:92
>> +            cookiesBuilder.appendLiteral("; ");
> 
> I think a cleaner way to write this is to put at the beginning of the loop:
> 
>     if (!cookiesBuilder.isEmpty())
>         cookiesBuilder.appendLiteral("; ");
> 
> Then we don’t need lastCookie.

Oh right, better indeed, thanks.
Comment 5 Chris Dumez 2014-10-19 20:55:33 PDT
Created attachment 240097 [details]
Patch
Comment 6 WebKit Commit Bot 2014-10-20 00:09:43 PDT
Comment on attachment 240097 [details]
Patch

Clearing flags on attachment: 240097

Committed r174877: <http://trac.webkit.org/changeset/174877>
Comment 7 WebKit Commit Bot 2014-10-20 00:09:48 PDT
All reviewed patches have been landed.  Closing bug.
Comment 8 Alexey Proskuryakov 2014-10-24 12:42:36 PDT
This caused bug 138053.