1362822 - Unnecessary locking cost during the creation of HTTP channels

Status: RESOLVED FIXED

(Core :: Networking: HTTP, enhancement)

Description

[(no longer active)]

See this perf profile:

   - 5.65% imgLoader::LoadImage                                                                       ▒
      + 3.06% mozilla::net::HttpChannelChild::AsyncOpen2                                              ▒
      - 1.50% NewImageChannel                                                                         ▒
         - 0.96% NS_NewChannelWithTriggeringPrincipal                                                 ▒
              NS_NewChannelInternal                                                                   ▒
              mozilla::net::nsIOService::NewChannelFromURI2                                           ▒
            - mozilla::net::nsIOService::NewChannelFromURIWithProxyFlags2                             ▒
               - 0.43% mozilla::net::nsIOService::NewChannelFromURIWithProxyFlagsInternal             ◆
                  - mozilla::net::nsHttpHandler::NewProxiedChannel2                                   ▒
                     - 0.14% mozilla::net::HttpBaseChannel::Init                                      ▒
                        - 0.07% mozilla::net::nsHttpHandler::AddStandardRequestHeaders                ▒
                             0.05% pthread_mutex_unlock                                               ▒
                           - 0.02% mozilla::net::nsHttpRequestHead::SetHeader                         ▒
                                mozilla::net::nsHttpHeaderArray::SetHeader_internal                   ▒
                                nsTArray_Impl<mozilla::net::nsHttpHeaderArray::nsEntry, nsTArrayInfall▒
                                nsTArray_base<nsTArrayInfallibleAllocator, nsTArray_CopyWithMemutils>:▒
                                moz_xrealloc                                                          ▒
                                pthread_mutex_unlock                                                  ▒

A lot of the cost of HttpBaseChannel::Init() is coming from the locking cost from the SetHeader calls under nsHttpHandler::AddStandardRequestHeaders().  This is quite silly because we are just creating the channel object, and there is yet no other consumer to protect against.

Comment 1

[(no longer active)]

Attachment: Avoid needlessly holding locks when setting headers during the initialization of HTTP channel objects

The channel objects cannot be handed off to other threads before the creation
process has been finished, so there is no point in trying to hold these locks
while the initialization code is running.  These lockings have shown up in
profiles as being expensive.

Comment 2

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8865180 [details] [diff] [review]
Avoid needlessly holding locks when setting headers during the initialization of HTTP channel objects

Review of attachment 8865180 [details] [diff] [review]:
-----------------------------------------------------------------

Another way to do it is to call |nsHttpHeaderArray::SetHeader| directly.
We can either use the nsHttpRequestHead::Headers to modify the header array in place, or write into a local nsHttpHeaderArray then replace the one in nsHttpRequestHead.

@macmanus, which approach you prefer? create non-thread-safe function in nsHttpRequestHead or modify nsHttpHeaderArray directly.

::: netwerk/protocol/http/nsHttpRequestHead.h
@@ +114,5 @@
> +    // This function is designed to only be called during the
> +    // creation of this object, and doesn't grab the lock of the
> +    // object before setting the header.  Please do not use it if
> +    // you are not sure why you need it.
> +    MOZ_MUST_USE nsresult SetHeaderNonThreadSafe(nsHttpAtom h,

I'd like to name it |InitHeaderNonThreadSafe| to explicitly state the usage in initialization phase.

Comment 3

[Patrick McManus [:mcmanus]]

dragana is the expert here - she should decide

Comment 4

[Dragana Damjanovic [:dragana]]

Comment on attachment 8865180 [details] [diff] [review]
Avoid needlessly holding locks when setting headers during the initialization of HTTP channel objects

Review of attachment 8865180 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/protocol/http/nsHttpHandler.cpp
@@ +529,5 @@
>      return rv;
>  }
>  
>  nsresult
>  nsHttpHandler::AddStandardRequestHeaders(nsHttpRequestHead *request, bool isSecure)

Can you please add a scary comment (that says that headers are not lock and that it is allowed to use this function only in HttpBaseChannel::init) above declaration of this function as well. I am afraid that someone will decide to use this function outside of nsHttpChannel::Init.

Comment 5

[Honza Bambas (:mayhemer)]

Should there be a debug flag on nsHttpRequestHead we drop at the end of HttpBaseChannel::init to have a nice assert in SetHeaderNonThreadSafe?

Comment 6

[Shih-Chiang Chien [:schien] (UTC+8) (use ni? plz)]

Comment on attachment 8865180 [details] [diff] [review]
Avoid needlessly holding locks when setting headers during the initialization of HTTP channel objects

Review of attachment 8865180 [details] [diff] [review]:
-----------------------------------------------------------------

r+'ed with @dragana's comment addressed.

Comment 7

[(no longer active)]

I will add the comments, but I think Honza is right, an assertion would be much better!  I'll add one.

Comment 8

[(no longer active)]

Attachment: follow-up: Address the review comments and add the suggested assertion

I think this got complex enough to merit a review of its own.  I'm planning to fold this into the previous patch when landing, so apologies for the messy commit message and the diff, but this is the changes that I made on top of what you reviewed previously, so that you don't have to spend time looking over the same changes twice!

(If you prefer the assertion part to be landed as a separate patch please let me know, I can move the added comment to the previous part...)

Comment 9

[Dragana Damjanovic [:dragana]]

Comment on attachment 8866596 [details] [diff] [review]
follow-up: Address the review comments and add the suggested assertion

Review of attachment 8866596 [details] [diff] [review]:
-----------------------------------------------------------------

Thank you!

Comment 10

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/0a25a0fa3b32
Avoid needlessly holding locks when setting headers during the initialization of HTTP channel objects; r=schien,dragana

Comment 11

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

This appears to have caused osx debug devtools tests to time out like https://treeherder.mozilla.org/logviewer.html#?job_id=98380678&repo=mozilla-inbound


https://treeherder.mozilla.org/#/jobs?repo=mozilla-inbound&noautoclassify&fromchange=b99e360725c87de4b8cf8e537dedc340fb2d4bed&group_state=expanded&filter-searchStr=10.%20debug%20dt&tochange=65684cf111ae133bbd1a2b010fd232d7ba0297f3


Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/50940861284c

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Hmm, that was probably bug 1363893. I'll get this relanded later today.

Comment 13

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/35f21a51a75b
Avoid needlessly holding locks when setting headers during the initialization of HTTP channel objects; r=schien,dragana

Comment 14

[Pulsebot]

Backout by [e-mail]:
https://hg.mozilla.org/mozilla-central/rev/2a8e0c4be57d
Backed out changeset 0a25a0fa3b32 for osx debug devtools timeouts a=merge

Comment 15

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/35f21a51a75b