Closed
Bug 760380
Opened 12 years ago
Closed 12 years ago
Stop calling mOfflineCacheEntry->GetLastModified when processing response from normal cache
Categories
(Core :: Networking: HTTP, defect, P1)
Core
Networking: HTTP
Tracking
()
RESOLVED
FIXED
mozilla16
People
(Reporter: briansmith, Assigned: briansmith)
References
Details
(Keywords: main-thread-io, perf)
Attachments
(1 file, 1 obsolete file)
+++ This bug was initially created as a clone of Bug #722034 +++ This was Part 7 of the series of patches in Bug #722034. Since it wasn't strictly necessary, and we needed to land that series ASAP, I moved this patch to this new bug. (In reply to Brian Smith (:bsmith) from comment #58) > Created attachment 628515 [details] [diff] [review] > Part 7 - Stop calling mOfflineCacheEntry->GetLastModified when processing > response from normal cache, r?honzab > > This patch simply moves the call to mOfflineCacheEntry->GetLastModified to > OnOfflineCacheEntryForWritingAvailable, to remove the last non-error, > pre-validation access to cache entries from the processing of normal cache > entries. We We will still do parts of the offline cache entry processing on > the the main thread temporarily (see bug 759886 and bug 759889) but this is > the best place to put this access for now. (In reply to Brian Smith (:bsmith) from comment #68) > (In reply to Honza Bambas (:mayhemer) from comment #66) > > Comment on attachment 628515 [details] [diff] [review] > > Part 7 - Stop calling mOfflineCacheEntry->GetLastModified when processing > > response from normal cache, r?honzab > > > > Why is this patch needed? > > I tried to explain in Bug 722034 comment 58. It isn't strictly needed, > because I am just moving it from one place executed on the main thread to > another place executed on the main thread. But, long term, we cannot call > mOfflineCacheEntry->GetLastModified in ShouldUpdateOfflineCachEntry because > ShouldUpdateOfflineCachEntry will always be called on the main thread, and > mOfflineCacheEntry->GetLastModified takes the cache service lock. And, after > bug 759889 is fixed, we will stop calling > nsHttpChannel::OnOfflineCacheEntryForWritingAvailable on the main thread.
Attachment #629088 -
Flags: review?(honzab.moz)
Assignee | ||
Updated•12 years ago
|
Assignee: nobody → bsmith
Comment 1•12 years ago
|
||
Comment on attachment 629088 [details] [diff] [review] Stop calling mOfflineCacheEntry->GetLastModified when processing response from normal cache Review of attachment 629088 [details] [diff] [review]: ----------------------------------------------------------------- The logic looks good, but... where do you read mOfflineCacheLastModifiedTime ? The patch seems incomplete.
Assignee | ||
Comment 2•12 years ago
|
||
(In reply to Honza Bambas (:mayhemer) from comment #1) > The logic looks good, but... where do you read mOfflineCacheLastModifiedTime > ? The patch seems incomplete. In nsHttpChannel::ShouldUpdateOfflineCacheEntry.
Comment 3•12 years ago
|
||
(In reply to Brian Smith (:bsmith) from comment #2) > (In reply to Honza Bambas (:mayhemer) from comment #1) > > The logic looks good, but... where do you read mOfflineCacheLastModifiedTime > > ? The patch seems incomplete. > > In nsHttpChannel::ShouldUpdateOfflineCacheEntry. But I cannot see it in the patch. Is in some followup patch? It doesn't make sense to separate things like this, have a patch that introduces and checks a new member, but doesn't set it.
Comment 4•12 years ago
|
||
Comment on attachment 629088 [details] [diff] [review] Stop calling mOfflineCacheEntry->GetLastModified when processing response from normal cache Please re-request after I get answer to comment 3.
Attachment #629088 -
Flags: review?(honzab.moz)
Assignee | ||
Comment 5•12 years ago
|
||
Comment on attachment 629088 [details] [diff] [review] Stop calling mOfflineCacheEntry->GetLastModified when processing response from normal cache Review of attachment 629088 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/protocol/http/nsHttpChannel.cpp @@ +3263,4 @@ > return false; > } > > + if (docLastModifiedTime > mOfflineCacheLastModifiedTime) { mOfflineCacheLastModifiedTime is used right here.
Assignee | ||
Updated•12 years ago
|
Attachment #629088 -
Flags: review?(honzab.moz)
Comment 6•12 years ago
|
||
(In reply to Brian Smith (:bsmith) from comment #5) > mOfflineCacheLastModifiedTime is used right here. Where is it set?
Assignee | ||
Comment 7•12 years ago
|
||
Sorry Honza, here's the fixed version of the patch.
Attachment #629088 -
Attachment is obsolete: true
Attachment #629088 -
Flags: review?(honzab.moz)
Attachment #632359 -
Flags: review?(honzab.moz)
Comment 8•12 years ago
|
||
Comment on attachment 632359 [details] [diff] [review] Stop calling mOfflineCacheEntry->GetLastModified when processing response from normal cache [v2] Review of attachment 632359 [details] [diff] [review]: ----------------------------------------------------------------- r=honzab with just a nit: Please initialize the new member also in the constructor to 0.
Attachment #632359 -
Flags: review?(honzab.moz) → review+
Assignee | ||
Comment 9•12 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/5030b469ca79
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla16
You need to log in
before you can comment on or make changes to this bug.
Description
•