Closed
Bug 1325091
Opened 8 years ago
Closed 8 years ago
Add flag indicating presence of alternative data in the cache entry to cache index
Categories
(Core :: Networking: Cache, defect)
Core
Networking: Cache
Tracking
()
RESOLVED
FIXED
mozilla55
Tracking | Status | |
---|---|---|
firefox55 | --- | fixed |
People
(Reporter: michal, Assigned: CuveeHsu)
References
(Blocks 1 open bug)
Details
(Whiteboard: [necko-active])
Attachments
(1 file, 4 obsolete files)
28.51 KB,
patch
|
michal
:
review+
|
Details | Diff | Splinter Review |
If there is an alternative data representation in the cache entry, we should prefer opening the entry to reading the content from network. The information must be present in the cache index so we have it available when deciding whether to race cache with the network.
Reporter | ||
Updated•8 years ago
|
Assignee: nobody → michal.novotny
Whiteboard: [necko-active]
Assignee | ||
Comment 2•8 years ago
|
||
We need a flag in CacheIndexRecord.
I'd like to add another member mHasCachedAltData instead of using reserved bits in mFlags.
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#81
The reason is not to mess those bit magic like here:
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#385
Do you agree, Michal?
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 3•8 years ago
|
||
Another drawback of additional member is wasting space for buffer like this:
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#234
Reporter | ||
Comment 4•8 years ago
|
||
(In reply to Junior[:junior] from comment #2)
> We need a flag in CacheIndexRecord.
> I'd like to add another member mHasCachedAltData instead of using reserved
> bits in mFlags.
> http://searchfox.org/mozilla-central/rev/
> 39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#81
>
> The reason is not to mess those bit magic like here:
> http://searchfox.org/mozilla-central/rev/
> 39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#385
>
> Do you agree, Michal?
No, it would waste memory as well as disk space.
Flags: needinfo?(michal.novotny)
Assignee | ||
Comment 5•8 years ago
|
||
Assignee | ||
Comment 6•8 years ago
|
||
Attachment #8841464 -
Flags: review?(michal.novotny)
Assignee | ||
Updated•8 years ago
|
Attachment #8841464 -
Attachment description: refactor-add-flag-to-index-v1 → Part1: refactor-add-flag-to-index-v1
Assignee | ||
Comment 7•8 years ago
|
||
Comment on attachment 8841464 [details] [diff] [review]
Part1: refactor-add-flag-to-index-v1
Review of attachment 8841464 [details] [diff] [review]:
-----------------------------------------------------------------
::: netwerk/cache2/CacheIndex.h
@@ +395,5 @@
> }
> aDst->mRec->mOriginAttrsHash = mRec->mOriginAttrsHash;
> + bool shouldToggleHasCachedAltData =
> + !!(mUpdateFlags & kHasCachedAltDataUpdatedMask) &&
> + !!((aDst->mRec->mFlags ^ mRec->mFlags) & kHasCachedAltDataMask);
Here's a bug.
We always update the flags to aDst->mRec in the next |if| block.
But we want to keep aDst->mRec->mFlags & kHasCachedAltDataMask the same if we don't want to update.
i.e., we should toggle back if we don't need to update
propose to change:
1) change to !(mUpdateFlags & kHasCachedAltDataUpdatedMask) &&
2) change the name to shouldToggleBackHasCachedAltData
Assignee | ||
Comment 8•8 years ago
|
||
1. Fix the bug addressed in last comment
2. Fix warning and nit
Attachment #8841464 -
Attachment is obsolete: true
Attachment #8841464 -
Flags: review?(michal.novotny)
Attachment #8842311 -
Flags: review?(michal.novotny)
Reporter | ||
Comment 9•8 years ago
|
||
Comment on attachment 8842311 [details] [diff] [review]
Part1: refactor-add-flag-to-index-v2
Review of attachment 8842311 [details] [diff] [review]:
-----------------------------------------------------------------
The alt-data flag isn't set anywhere.
::: netwerk/cache2/CacheIndex.h
@@ +208,5 @@
> mRec->mExpirationTime = aExpirationTime;
> }
> uint32_t GetExpirationTime() const { return mRec->mExpirationTime; }
>
> + void SetHasCachedAltData(bool aHasCachedAltData)
The name is unnecessarily long. HasAltData instead of HasCachedAltData would be sufficient.
@@ +397,5 @@
> aDst->mRec->mOriginAttrsHash = mRec->mOriginAttrsHash;
> +
> + bool shouldToggleBackHasCachedAltData =
> + !(mUpdateFlags & kHasCachedAltDataUpdatedMask) &&
> + !!((aDst->mRec->mFlags ^ mRec->mFlags) & kHasCachedAltDataMask);
This is hard to read. It would be better to copy the flag here if it was updated and exclude tha flag from copying below.
Attachment #8842311 -
Flags: review?(michal.novotny) → review-
Assignee | ||
Comment 10•8 years ago
|
||
(In reply to Michal Novotny (:michal) from comment #9)
> Comment on attachment 8842311 [details] [diff] [review]
> Part1: refactor-add-flag-to-index-v2
>
> Review of attachment 8842311 [details] [diff] [review]:
> -----------------------------------------------------------------
>
> The alt-data flag isn't set anywhere.
>
I should have mentioned that the flag setting would be in the next patch.
I'll upload both patches for review later.
Assignee | ||
Comment 11•8 years ago
|
||
I'm kinda worried that we never touch [1] since we will recreate a CacheEntry and openOutputStream to the brand new CacheFile.
I use the patch in bug 1347470 to test the overwrite behaviour.
[1] http://searchfox.org/mozilla-central/rev/ca7015fa45b30b29176fbaa70ba0a36fe9263c38/netwerk/cache2/CacheFile.cpp#892
Attachment #8842311 -
Attachment is obsolete: true
Attachment #8847581 -
Flags: review?(michal.novotny)
Reporter | ||
Comment 12•8 years ago
|
||
Comment on attachment 8847581 [details] [diff] [review]
add-flag-to-index-v3
Review of attachment 8847581 [details] [diff] [review]:
-----------------------------------------------------------------
You also need to set the flag when we parse the metadata when updating/building index. See CacheIndex::InitEntryFromDiskData. And don't forget to increase index version.
::: netwerk/cache2/CacheFile.cpp
@@ +1205,5 @@
> + PostWriteTimer();
> +
> + bool hasAltData = aAltMetadata ? true : false;
> + if (mHandle && !mHandle->IsDoomed())
> + CacheFileIOManager::UpdateIndexEntry(mHandle, nullptr, nullptr, &hasAltData);
Update the index entry after you set the metadata. If it fails you need to change the value accordingly. Also enclose it in curly braces.
::: netwerk/cache2/CacheFileIOManager.cpp
@@ +2859,5 @@
> // Move the entry at the end of both lists to make sure we won't end up
> // failing on one entry forever.
> uint32_t frecency = 0;
> uint32_t expTime = nsICacheEntry::NO_EXPIRATION_TIME;
> + bool hasAltData = false;
Why you drop the flag here?
Attachment #8847581 -
Flags: review?(michal.novotny) → feedback+
Reporter | ||
Comment 13•8 years ago
|
||
(In reply to Junior[:junior] from comment #11)
> I'm kinda worried that we never touch [1] since we will recreate a
> CacheEntry and openOutputStream to the brand new CacheFile.
And what's the problem? We need to handle it somehow. If we're sure CacheFile::OpenOutputStream is never called when there is some alt-data we could turn it into precondition and fail, but why to be restrictive when we don't need to?
Assignee | ||
Comment 14•8 years ago
|
||
> You also need to set the flag when we parse the metadata when
> updating/building index. See CacheIndex::InitEntryFromDiskData. And don't
> forget to increase index version.
>
Thanks for pointing this.
> ::: netwerk/cache2/CacheFileIOManager.cpp
> @@ +2859,5 @@
> > // Move the entry at the end of both lists to make sure we won't end up
> > // failing on one entry forever.
> > uint32_t frecency = 0;
> > uint32_t expTime = nsICacheEntry::NO_EXPIRATION_TIME;
> > + bool hasAltData = false;
>
> Why you drop the flag here?
I miss the annotation. Will revert the change.
> > I'm kinda worried that we never touch [1] since we will recreate a
> > CacheEntry and openOutputStream to the brand new CacheFile.
>
> And what's the problem? We need to handle it somehow. If we're sure
> CacheFile::OpenOutputStream is never called when there is some alt-data we
> could turn it into precondition and fail, but why to be restrictive when we
> don't need to?
Just afraid of wrong understanding. Also dead code always misleads engineers.
But I'm fine for adding this precondition :)
Assignee | ||
Comment 15•8 years ago
|
||
> > > I'm kinda worried that we never touch [1] since we will recreate a
> > > CacheEntry and openOutputStream to the brand new CacheFile.
> >
> > And what's the problem? We need to handle it somehow. If we're sure
> > CacheFile::OpenOutputStream is never called when there is some alt-data we
> > could turn it into precondition and fail, but why to be restrictive when we
> > don't need to?
>
> Just afraid of wrong understanding. Also dead code always misleads engineers.
> But I'm fine for adding this precondition :)
But it's not a strong condition we should make sure.
I won't do it in this patch.
Assignee | ||
Comment 16•8 years ago
|
||
Attachment #8847581 -
Attachment is obsolete: true
Attachment #8848996 -
Flags: review?(michal.novotny)
Assignee | ||
Comment 17•8 years ago
|
||
Reporter | ||
Comment 18•8 years ago
|
||
Comment on attachment 8848996 [details] [diff] [review]
add-alt-flat-to-index-v4
Review of attachment 8848996 [details] [diff] [review]:
-----------------------------------------------------------------
You didn't bump the index version.
::: netwerk/cache2/CacheFile.cpp
@@ +955,5 @@
>
> nsAutoCString altMetadata;
> CacheFileUtils::BuildAlternativeDataInfo(aAltDataType, mAltDataOffset,
> altMetadata);
> + SetAltMetadata(altMetadata.get());
You need to fail here if SetAltMetadata fails.
::: netwerk/cache2/CacheIndex.cpp
@@ +2664,5 @@
> aMetaData->GetFrecency(&frecency);
> aEntry->SetFrecency(frecency);
>
> + bool hasAltData = aMetaData->GetElement(CacheFileUtils::kAltDataKey) ? true : false;
> + aEntry->SetHasAltData(hasAltData);
We should check whether we're able to parse the altdata info like we do in CacheFile::OnMetadataRead(). If it fails we should remove the file in CacheIndex::BuildIndex()/CacheIndex::UpdateIndex().
Attachment #8848996 -
Flags: review?(michal.novotny) → review-
Assignee | ||
Comment 19•8 years ago
|
||
Attachment #8848996 -
Attachment is obsolete: true
Attachment #8849494 -
Flags: review?(michal.novotny)
Reporter | ||
Updated•8 years ago
|
Attachment #8849494 -
Flags: review?(michal.novotny) → review+
Assignee | ||
Comment 20•8 years ago
|
||
Assignee | ||
Updated•8 years ago
|
Keywords: checkin-needed
Comment 21•8 years ago
|
||
Pushed by ryanvm@gmail.com:
https://hg.mozilla.org/integration/mozilla-inbound/rev/10514ae0b128
Add flag indicating presence of alternative data in the cache entry to cache index. r=michal
Keywords: checkin-needed
Comment 22•8 years ago
|
||
bugherder |
Status: NEW → RESOLVED
Closed: 8 years ago
status-firefox55:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Updated•8 years ago
|
status-firefox53:
affected → ---
You need to log in
before you can comment on or make changes to this bug.
Description
•