Closed Bug 1136897 Opened 10 years ago Closed 10 years ago

Stop evicting expired disk entries

Categories

(Core :: Networking: Cache, defect)

defect
Not set
major

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: mayhemer, Assigned: mayhemer)

References

Details

Attachments

(1 file, 5 obsolete files)

Since this is probably causing problems such as bug 1060082 and is only partially fixed with bug 1079789, I think we should do a two stage fix: In this bug stop evicting disk entries based on expiration at all. When a significant(ish) regression occurs, file a followup bug to evict expired entries that cannot be reused (missing validation info). I want to move on here since the validation information must be stored in the index. It's planned for a different performance feature (that is not moving on right now). But this is a functionality issue. Hence - proceed with a simple as-good solution.
Attached patch v1 (obsolete) — Splinter Review
This is a quick fix which is also upliftable. Manually tested. https://treeherder.mozilla.org/#/jobs?repo=try&revision=fc28252dea7b
Attachment #8569459 - Flags: review?(michal.novotny)
Comment on attachment 8569459 [details] [diff] [review] v1 Review of attachment 8569459 [details] [diff] [review]: ----------------------------------------------------------------- There is no reason to keep mExpirationArray since this was the only usage. Please comment out also mExpirationArray together with InsertRecordToExpirationArray() and RemoveRecordFromExpirationArray() methods.
Attachment #8569459 - Flags: review?(michal.novotny) → review-
I expected that :D
Michal, would you be OK with removing the code and not just commenting out? There is a lot of code, I started to #ifdef CACHE_DOOM_EXPIRED it, but it's kinda messy.
Flags: needinfo?(michal.novotny)
Sure, we can always revert the change if we find out that we need it.
Flags: needinfo?(michal.novotny)
Attached patch v2 (obsolete) — Splinter Review
Attachment #8569459 - Attachment is obsolete: true
Attachment #8570549 - Flags: review?(michal.novotny)
Attached patch v2 -w (for easier review) (obsolete) — Splinter Review
Comment on attachment 8570549 [details] [diff] [review] v2 Review of attachment 8570549 [details] [diff] [review]: ----------------------------------------------------------------- Expiration time in CacheIndex is used only for eviction purposes. When removing mExpirationArray we should also remove expiration time from CacheIndexRecord etc. I'm OK with doing it in a follow up bug. ::: netwerk/cache2/CacheIndex.cpp @@ +35,5 @@ > namespace mozilla { > namespace net { > > /** > * This helper class is responsible for keeping CacheIndex::mIndexStats, Replace the comma with "and". @@ -97,5 @@ > - } else { > - if (entry->mRec->mFrecency != mOldFrecency) { > - replaceFrecency = true; > - } > - if (entry->mRec->mExpirationTime != mOldExpirationTime) { There is no other use for mOldExpirationTime, remove it completely. @@ +1176,3 @@ > SHA1Sum::Hash hash; > bool foundEntry = false; > uint32_t i = 0, j = 0; Variable i is unused, remove it. @@ +1197,5 @@ > > + if (!foundEntry) > + return NS_ERROR_NOT_AVAILABLE; > + > + // forced valid entries skipped in both arrays could overlap, just use max The comment is no longer valid. @@ +1198,5 @@ > + if (!foundEntry) > + return NS_ERROR_NOT_AVAILABLE; > + > + // forced valid entries skipped in both arrays could overlap, just use max > + *aCnt = index->mFrecencyArray.Length() - std::max(i, j); i is unused, return simply mFrecencyArray.Length() - j @@ +1204,5 @@ > + LOG(("CacheIndex::GetEntryForEviction() - returning entry from frecency " > + "array [hash=%08x%08x%08x%08x%08x, cnt=%u, expTime=%u, now=%u, " > + "frecency=%u]", LOGSHA1(&hash), *aCnt, > + index->mFrecencyArray[j]->mExpirationTime, now, > + index->mFrecencyArray[j]->mFrecency)); It might not worth to log expiration time anymore when we don't use it to determine which entry to evict. @@ -3293,5 @@ > - LOG(("CacheIndex::InsertRecordToExpirationArray() [record=%p, hash=%08x%08x" > - "%08x%08x%08x]", aRecord, LOGSHA1(aRecord->mHash))); > - > - MOZ_ASSERT(!mExpirationArray.Contains(aRecord)); > - mExpirationArray.InsertElementSorted(aRecord, ExpirationComparator()); Remove also ExpirationComparator() function. ::: netwerk/cache2/CacheIndex.h @@ +920,2 @@ > void RemoveRecordFromFrecencyArray(CacheIndexRecord *aRecord); > void RemoveRecordFromExpirationArray(CacheIndexRecord *aRecord); Remove RemoveRecordFromExpirationArray declaration too. @@ +1022,2 @@ > > // Arrays that keep entry records ordered by eviction preference. When looking Please change this comment accordingly.
Attachment #8570549 - Flags: review?(michal.novotny) → review-
Comment on attachment 8570549 [details] [diff] [review] v2 Review of attachment 8570549 [details] [diff] [review]: ----------------------------------------------------------------- ::: netwerk/cache2/CacheIndex.cpp @@ -97,5 @@ > - } else { > - if (entry->mRec->mFrecency != mOldFrecency) { > - replaceFrecency = true; > - } > - if (entry->mRec->mExpirationTime != mOldExpirationTime) { except storing to the index. Let's leave it for now (see the previous comment), otherwise it would mean to change the index format (or store meaningless data in it.)
(In reply to Honza Bambas (:mayhemer) from comment #9) > except storing to the index. Let's leave it for now (see the previous > comment), otherwise it would mean to change the index format (or store > meaningless data in it.) mOldExpirationTime! Yep, it makes sense.
Attached patch v2.1 (obsolete) — Splinter Review
Attachment #8570549 - Attachment is obsolete: true
Attachment #8570550 - Attachment is obsolete: true
Attachment #8571446 - Flags: review?(michal.novotny)
Comment on attachment 8571446 [details] [diff] [review] v2.1 Review of attachment 8571446 [details] [diff] [review]: ----------------------------------------------------------------- r+ with the following fixed ::: netwerk/cache2/CacheIndex.cpp @@ +1199,5 @@ > + > + LOG(("CacheIndex::GetEntryForEviction() - returning entry from frecency " > + "array [hash=%08x%08x%08x%08x%08x, cnt=%u, now=%u, " > + "frecency=%u]", LOGSHA1(&hash), *aCnt, now, > + index->mFrecencyArray[i]->mFrecency)); Please remove also logging of now and remove variable now too. ::: netwerk/cache2/CacheIndex.h @@ +1021,3 @@ > > // Arrays that keep entry records ordered by eviction preference. When looking > // for an entry to evict, we first try to find an expired entry. If there is You forgot to change this comment. Arrays -> Array remove the part about expired entries
Attachment #8571446 - Flags: review?(michal.novotny) → review+
Attached patch v2.2 (obsolete) — Splinter Review
Waiting with c-i before some try failures are resolved.
Attachment #8571980 - Flags: review+
Attached patch v2.3Splinter Review
Attachment #8571446 - Attachment is obsolete: true
Attachment #8571980 - Attachment is obsolete: true
Attachment #8571996 - Flags: review+
Blocks: 1079789
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: