Bug 93731 - Use static AtomicStrings for HTTP header field names
Summary: Use static AtomicStrings for HTTP header field names
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Nobody
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2012-08-10 11:38 PDT by Yong Li
Modified: 2014-06-19 10:25 PDT (History)
3 users (show)

See Also:


Attachments
Not ready for review yet. (1021 bytes, patch)
2012-08-10 11:38 PDT, Yong Li
no flags Details | Formatted Diff | Diff
not ready for review (9.96 KB, patch)
2012-08-10 11:39 PDT, Yong Li
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Yong Li 2012-08-10 11:38:10 PDT
Eevery call to m_httpHeaderFields.contains(<const c string>) constructs a new WTF::String, and calculate its hash again. 

This can be avoided by using static Strings.

I have a patch for it. I haven't been able to tell how much it helps, but theoretically it saves CPU time on string constructing and hashing...
Comment 1 Yong Li 2012-08-10 11:38:53 PDT
Created attachment 157772 [details]
Not ready for review yet.
Comment 2 Yong Li 2012-08-10 11:39:14 PDT
Comment on attachment 157772 [details]
Not ready for review yet.

oops. not this one
Comment 3 Yong Li 2012-08-10 11:39:52 PDT
Created attachment 157774 [details]
not ready for review
Comment 4 Yong Li 2012-08-10 11:40:57 PDT
is it worth doing?
Comment 5 Alexey Proskuryakov 2012-08-10 13:13:52 PDT
Comment on attachment 157774 [details]
not ready for review

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

I don't know. Probably not much harm, and not much good.

Note however that the current patch is strictly a regression. These AtomicStrings have a regular hash pre-calculated, but HashMap uses CaseFoldingHash. So, the pre-caclculated hash will not be used.

Generally, it's much easier to start with profiler data and optimize that's taking significant time than to figure out whether a random change improves performance.

> Source/WebCore/platform/network/ResourceRequestBase.cpp:43
> +AtomicString* ResourceRequestBase::headerNameAccept = new AtomicString("Accept");

This is not OK - we can not have global with initializers. No code should be run when WebKit shared library loads.

You'll probably need a new file similar to HTMLNames.h.
Comment 6 Yong Li 2012-08-10 13:33:26 PDT
(In reply to comment #5)
> (From update of attachment 157774 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=157774&action=review
> 
> I don't know. Probably not much harm, and not much good.
> 
> Note however that the current patch is strictly a regression. These AtomicStrings have a regular hash pre-calculated, but HashMap uses CaseFoldingHash. So, the pre-caclculated hash will not be used.

Bang! I forgot this..

> 
> > Source/WebCore/platform/network/ResourceRequestBase.cpp:43
> > +AtomicString* ResourceRequestBase::headerNameAccept = new AtomicString("Accept");
> 
> This is not OK - we can not have global with initializers. No code should be run when WebKit shared library loads.
> 
> You'll probably need a new file similar to HTMLNames.h.

Thanks!
Comment 7 Anders Carlsson 2014-06-18 12:38:15 PDT
This has been fixed; we now use an enum for header names.
Comment 8 Alexey Proskuryakov 2014-06-18 21:07:25 PDT
An enum? I wasn't watching these patches carefully, how does that support arbitrary header field names (exposed via XMLHttpRequest)?
Comment 9 Anders Carlsson 2014-06-19 10:25:20 PDT
(In reply to comment #8)
> An enum? I wasn't watching these patches carefully, how does that support arbitrary header field names (exposed via XMLHttpRequest)?

We use an enum for the common header types and regular strings for other headers.