633146 - Remove unnecessary locking in nsCacheService::OpenCacheEntry

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Bjarne (:bjarne)]

Refer to bug #630420, comment #19

nsCacheService::OpenCachEntry() always grabs the cache-lock. This is only necessary if the request is handled synchronously, i.e. if ProcessRequest() is called from OpenCacheEntry().

Comment 1

[Bjarne (:bjarne)]

Attachment: Proposed solution

This should do the trick. I also cleaned up the code a little.

Passes local testing - pushed to tryserver.

Comment 2

[Bjarne (:bjarne)]

Attachment: Proposed solution v2

Hmm...  didn't do a qref... :(

Here is the cleaned-up code which also deletes the request if dispatching fails.

Comment 3

[Bjarne (:bjarne)]

Tryserver looks fine.

Comment 4

[Bjarne (:bjarne)]

Adding jst and jduell. This fix essentially eliminates one completely unnecessary lock/unlock pair from every channel-load.

Comment 5

[Johnny Stenback (:jst)]

Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

Given the low return from taking this and the potential for regressions, I'm hesitant to take this days before the last beta. If you think the gain here is large enough to warrant taking any risk here, please renominate.

Comment 6

[Bjarne (:bjarne)]

Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

I'm re-nominating this...  ;)

Plz refer to bug #630420, comment #37. This patch also avoids blocking main-thread on the cache-service mutex.

I'm also requesting an additional review by bz, asking particularly to assess the potential for regression.

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

Please re-request approval once this is fully reviewed.

Comment 8

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

As long as the various things the request creation gets off the cache session are either immutable or mutated right now without holding the cache service lock, this looks ok.

Comment 9

[Bjarne (:bjarne)]

Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

The request-object is a rather simple creature merely describing the request without referencing anything but the listener. Requesting approval for 2.0 again.

Comment 10

[Asa Dotzler [:asa]]

Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

not taking for Firefox 4. Please land when we open mozilla-central for post Firefox 4 changes.

Comment 11

[(no longer active)]

http://hg.mozilla.org/projects/cedar/rev/5df1de0e6a68

Comment 12

[(no longer active)]

http://hg.mozilla.org/mozilla-central/rev/5df1de0e6a68