Closed
Bug 1136897
Opened 10 years ago
Closed 10 years ago
Stop evicting expired disk entries
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: mayhemer, Assigned: mayhemer)
References
Details
Attachments
(1 file, 5 obsolete files)
13.61 KB,
patch
|
mayhemer
:
review+
|
Details | Diff | Splinter Review |
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.
![]() |
Assignee | |
Comment 1•10 years ago
|
||
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 2•10 years ago
|
||
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-
![]() |
Assignee | |
Comment 3•10 years ago
|
||
I expected that :D
![]() |
Assignee | |
Comment 4•10 years ago
|
||
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)
Comment 5•10 years ago
|
||
Sure, we can always revert the change if we find out that we need it.
Flags: needinfo?(michal.novotny)
![]() |
Assignee | |
Comment 6•10 years ago
|
||
Attachment #8569459 -
Attachment is obsolete: true
Attachment #8570549 -
Flags: review?(michal.novotny)
![]() |
Assignee | |
Comment 7•10 years ago
|
||
Comment 8•10 years ago
|
||
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-
![]() |
Assignee | |
Comment 9•10 years ago
|
||
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.)
![]() |
Assignee | |
Comment 10•10 years ago
|
||
(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.
![]() |
Assignee | |
Comment 11•10 years ago
|
||
Attachment #8570549 -
Attachment is obsolete: true
Attachment #8570550 -
Attachment is obsolete: true
Attachment #8571446 -
Flags: review?(michal.novotny)
Comment 12•10 years ago
|
||
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+
![]() |
Assignee | |
Comment 13•10 years ago
|
||
Waiting with c-i before some try failures are resolved.
Attachment #8571980 -
Flags: review+
![]() |
Assignee | |
Comment 14•10 years ago
|
||
![]() |
Assignee | |
Comment 15•10 years ago
|
||
Attachment #8571446 -
Attachment is obsolete: true
Attachment #8571980 -
Attachment is obsolete: true
Attachment #8571996 -
Flags: review+
![]() |
Assignee | |
Updated•10 years ago
|
Keywords: checkin-needed
Comment 16•10 years ago
|
||
Keywords: checkin-needed
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in
before you can comment on or make changes to this bug.
Description
•