Use static AtomicStrings for HTTP header field names

• Component: Page Loading
• Status: RESOLVED FIXED

[Yong Li]

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...

[Yong Li]

Created attachment 157772 [details]
Not ready for review yet.

[Yong Li]

Comment on attachment 157772 [details]
Not ready for review yet.

oops. not this one

[Yong Li]

Created attachment 157774 [details]
not ready for review

[Yong Li]

is it worth doing?

[Alexey Proskuryakov]

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.

[Yong Li]

(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!

[Anders Carlsson]

This has been fixed; we now use an enum for header names.

[Alexey Proskuryakov]

An enum? I wasn't watching these patches carefully, how does that support arbitrary header field names (exposed via XMLHttpRequest)?

[Anders Carlsson]

(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.