Closed Bug 633146 Opened 9 years ago Closed 9 years ago

Remove unnecessary locking in nsCacheService::OpenCacheEntry

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla5

People

(Reporter: bjarne, Assigned: bjarne)

Details

(Keywords: perf)

Attachments

(1 file, 1 obsolete file)

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().
Attached patch Proposed solution (obsolete) — Splinter Review
This should do the trick. I also cleaned up the code a little.

Passes local testing - pushed to tryserver.
Assignee: nobody → bjarne
Attachment #511345 - Flags: review?(michal.novotny)
Hmm...  didn't do a qref... :(

Here is the cleaned-up code which also deletes the request if dispatching fails.
Attachment #511345 - Attachment is obsolete: true
Attachment #511348 - Flags: review?(michal.novotny)
Attachment #511345 - Flags: review?(michal.novotny)
Tryserver looks fine.
Attachment #511348 - Flags: review?(michal.novotny) → review+
Attachment #511348 - Flags: approval2.0?
Adding jst and jduell. This fix essentially eliminates one completely unnecessary lock/unlock pair from every channel-load.
Status: NEW → ASSIGNED
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.
Attachment #511348 - Flags: approval2.0? → approval2.0-
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.
Attachment #511348 - Flags: review?(bzbarsky)
Attachment #511348 - Flags: approval2.0?
Attachment #511348 - Flags: approval2.0-
Comment on attachment 511348 [details] [diff] [review]
Proposed solution v2

Please re-request approval once this is fully reviewed.
Attachment #511348 - Flags: approval2.0?
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.
Attachment #511348 - Flags: review?(bzbarsky) → review+
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.
Attachment #511348 - Flags: approval2.0?
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.
Attachment #511348 - Flags: approval2.0? → approval2.0-
Keywords: perf
http://hg.mozilla.org/mozilla-central/rev/5df1de0e6a68
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Whiteboard: fixed-in-cedar
Target Milestone: --- → mozilla2.2
You need to log in before you can comment on or make changes to this bug.