Add flag indicating presence of alternative data in the cache entry to cache index

RESOLVED FIXED in Firefox 55

Status

()

defect
RESOLVED FIXED
3 years ago
2 years ago

People

(Reporter: michal, Assigned: junior)

Tracking

(Blocks 1 bug)

Trunk
mozilla55
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox55 fixed)

Details

(Whiteboard: [necko-active])

Attachments

(1 attachment, 4 obsolete attachments)

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.
Blocks: RCWN
Blocks: 1325341
Assignee: nobody → michal.novotny
Whiteboard: [necko-active]
steal assignee from michal
Assignee: michal.novotny → juhsu
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)
Another drawback of additional member is wasting space for buffer like this:
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#234
(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)
Attachment #8841464 - Flags: review?(michal.novotny)
Attachment #8841464 - Attachment description: refactor-add-flag-to-index-v1 → Part1: refactor-add-flag-to-index-v1
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
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)
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-
(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.
Posted patch add-flag-to-index-v3 (obsolete) — Splinter Review
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)
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+
(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?
> 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 :)
> > > 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.
Posted patch add-alt-flat-to-index-v4 (obsolete) — Splinter Review
Attachment #8847581 - Attachment is obsolete: true
Attachment #8848996 - Flags: review?(michal.novotny)
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-
Attachment #8848996 - Attachment is obsolete: true
Attachment #8849494 - Flags: review?(michal.novotny)
Attachment #8849494 - Flags: review?(michal.novotny) → review+
Keywords: checkin-needed
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
https://hg.mozilla.org/mozilla-central/rev/10514ae0b128
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1350256
Depends on: 1360163
You need to log in before you can comment on or make changes to this bug.