Bug 137869 – [Mac] Optimize cookiesForDOM() by filtering and serializing cookies in a single pass

• Component: DOM
• Status: RESOLVED FIXED

[Chris Dumez]

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

[Chris Dumez]

Created attachment 240091 [details]
Patch

[Chris Dumez]

Created attachment 240092 [details]
Patch

[Darin Adler]

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.

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 240097 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 240097 [details]
Patch

Clearing flags on attachment: 240097

Committed r174877: <http://trac.webkit.org/changeset/174877>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Alexey Proskuryakov]

This caused bug 138053.