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

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

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.

Comment 1

[Junior [inactive]]

steal assignee from michal

Comment 2

[Junior [inactive]]

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?

Comment 3

[Junior [inactive]]

Another drawback of additional member is wasting space for buffer like this:
http://searchfox.org/mozilla-central/rev/39e4b25a076c59d2e7820297d62319f167871449/netwerk/cache2/CacheIndex.h#234

Comment 4

[Michal Novotny [:michal]]

(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.

Comment 5

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=8d189ab3823428c899024cd2ca7ca88b4d9c1215

Comment 6

[Junior [inactive]]

Attachment: Part1: refactor-add-flag-to-index-v1

Comment 7

[Junior [inactive]]

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

Comment 8

[Junior [inactive]]

Attachment: Part1: refactor-add-flag-to-index-v2

1. Fix the bug addressed in last comment
2. Fix warning and nit

Comment 9

[Michal Novotny [:michal]]

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.

Comment 10

[Junior [inactive]]

(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.

Comment 11

[Junior [inactive]]

Attachment: add-flag-to-index-v3

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

Comment 12

[Michal Novotny [:michal]]

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?

Comment 13

[Michal Novotny [:michal]]

(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?

Comment 14

[Junior [inactive]]

> 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 :)

Comment 15

[Junior [inactive]]

> > > 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.

Comment 16

[Junior [inactive]]

Attachment: add-alt-flat-to-index-v4

Comment 17

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=45277d6f197f7f27c685accb1c9c2a15fc8566ad

Comment 18

[Michal Novotny [:michal]]

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().

Comment 19

[Junior [inactive]]

Attachment: add-alt-data-flag-to-index-V5

Comment 20

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=043394c96fe478bac66c24f6763bedb745984b41&selectedJob=85278713

Comment 21

[Pulsebot]

Pushed by [e-mail]:
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

Comment 22

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/10514ae0b128