1325088 - Add net-response-time-onstart/onstop to cache index

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Description

[Michal Novotny [:michal]]

Add response times introduced in bug 1313095 to cache index so we have them available in memory and we could use them when deciding whether race network with the cache.

Comment 1

[Junior [inactive]]

steal assignee from michal

Comment 2

[Junior [inactive]]

Hello Michal, I have some questions:
1. We need to save the nettime to CacheIndexRecord and provide some utilities in CacheIndex.
   We still need to save the nettime to CacheFileMetadata and provide utilities in CacheFile.
   Is that correct?

2. Do we need to save the nettime to cache entry whenever we get the valid net time?
i.e. http://searchfox.org/mozilla-central/rev/ac8a72f2d5204ced2004c93a9c193430c63a6a71/netwerk/protocol/http/nsHttpChannel.cpp#6770

3. Now we're using mCacheEntry->SetMetaDataElement("net-response-time-onstart", START_TIME)
   to save those data to CacheFileMetadata.
   I guess we should provide APIs like SetNetResponseTimes to CacheEntry, CacheFile and CacheFileMetadata.
   And use them instead of SetMetaDataElement.
   Is that correct?

Comment 3

[Michal Novotny [:michal]]

(In reply to Junior[:junior] from comment #2)
> Hello Michal, I have some questions:
> 1. We need to save the nettime to CacheIndexRecord and provide some
> utilities in CacheIndex.
>    We still need to save the nettime to CacheFileMetadata and provide
> utilities in CacheFile.
>    Is that correct?

Yes, the information in CacheIndex is a duplicate and the primary place is still in cache's metadata. Index can be discarded at any time and we need to be able to rebuild it.


> 2. Do we need to save the nettime to cache entry whenever we get the valid
> net time?
> i.e.
> http://searchfox.org/mozilla-central/rev/
> ac8a72f2d5204ced2004c93a9c193430c63a6a71/netwerk/protocol/http/nsHttpChannel.
> cpp#6770

We should get the valid network time only when we first fetch the entry.


> 3. Now we're using
> mCacheEntry->SetMetaDataElement("net-response-time-onstart", START_TIME)
>    to save those data to CacheFileMetadata.
>    I guess we should provide APIs like SetNetResponseTimes to CacheEntry,
> CacheFile and CacheFileMetadata.
>    And use them instead of SetMetaDataElement.
>    Is that correct?

You could also get the information from the metadata but having dedicated methods on CacheEntry will be probably cleaner.

Comment 4

[Junior [inactive]]

Attachment: nettime - WIP1

refactor for adding onstart/onstop parameters, and fixed some alt-data

Comment 5

[Junior [inactive]]

Hello Michal,
(a) I'd like to change all the onstart/onstop times to uint16_t since
  1) this will save much spaces and memory for CacheIndex and CacheFileMetaData
  2) If the network time is greater than 65535 ms, it's no reason to race cache with network.

  The change will affect on previous CacheFileMetadata format and I'll do some backward compatible.
  What do you think?

(b) What I left from the patch in Comment 4 is to expose setter/getter to CacheEntry,
    move the metadata setting to CacheFile and let them update the cache index from CacheFile.

    Is it more review-able to have one more patch for this?

Comment 6

[Junior [inactive]]

Attachment: Part1: add onstart/stop to cache index - v2

It's a jump ball to me that put a nullptr or a 0 in |CacheFile::InitIndexEntry| for onstart/stop time.

and I split to two patches since it's easier to upload another merged one.

Comment 7

[Junior [inactive]]

Attachment: Part2: expose_to_cache_entry - v1

I keep the uint64_t in HttpChannel::ReportNetVSCacheTelemetry for backward-compat.

Comment 8

[Michal Novotny [:michal]]

(In reply to Junior[:junior] from comment #5)
> Hello Michal,
> (a) I'd like to change all the onstart/onstop times to uint16_t since
>   1) this will save much spaces and memory for CacheIndex and
> CacheFileMetaData
>   2) If the network time is greater than 65535 ms, it's no reason to race
> cache with network.
> 
>   The change will affect on previous CacheFileMetadata format and I'll do
> some backward compatible.
>   What do you think?

I agree that for racing decision we don't need to keep times longer than 65535ms, but this can affect HTTP_NET_VS_CACHE_ probes a lot. I think we can store uint32_t in the index. Or we could keep full time in metadata which would be used for telemetry and store uint16_t in the index which would be used only for racing decision.

Comment 9

[Michal Novotny [:michal]]

Comment on attachment 8851530 [details] [diff] [review]
Part1: add onstart/stop to cache index - v2

Review of attachment 8851530 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFile.cpp
@@ +2393,5 @@
>  
> +  const char *altData = mMetadata->GetElement(CacheFileUtils::kAltDataKey);
> +  bool hasAltData = altData ? true : false;
> +  if (hasAltData &&
> +      NS_FAILED(CacheFileUtils::ParseAlternativeDataInfo(altData, nullptr, nullptr))) {

It shouldn't be needed because when the entry is read from the disk, it is checked in CacheFile::OnMetadataRead(). If it is not read from the disk, no sanity check is needed because if there is any alt-data info we just set it on the newly created entry.

::: netwerk/cache2/CacheIndex.cpp
@@ +2707,5 @@
>    const char *altData = aMetaData->GetElement(CacheFileUtils::kAltDataKey);
>    bool hasAltData = altData ? true : false;
>    if (hasAltData &&
>        NS_FAILED(CacheFileUtils::ParseAlternativeDataInfo(altData, nullptr, nullptr))) {
> +      aMetaData->SetElement(CacheFileUtils::kAltDataKey, nullptr);

This shouldn't be needed because the entry will be removed.

@@ +2713,5 @@
>    }
>    aEntry->SetHasAltData(hasAltData);
>  
> +  const char *onStartTimeStr = aMetaData->GetElement("net-response-time-onstart");
> +  uint16_t onStartTime = onStartTimeStr ? (uint16_t) atoi(onStartTimeStr) : 0;

This will do modulo 65536 if the value is bigger than uint16_t.

::: netwerk/cache2/CacheIndex.h
@@ +270,5 @@
>      memcpy(ptr, mRec->mHash, sizeof(SHA1Sum::Hash)); ptr += sizeof(SHA1Sum::Hash);
>      NetworkEndian::writeUint32(ptr, mRec->mFrecency); ptr += sizeof(uint32_t);
>      NetworkEndian::writeUint64(ptr, mRec->mOriginAttrsHash); ptr += sizeof(uint64_t);
>      NetworkEndian::writeUint32(ptr, mRec->mExpirationTime); ptr += sizeof(uint32_t);
> +    NetworkEndian::writeInt16(ptr, mRec->mOnStartTime); ptr += sizeof(uint16_t);

writeUint16

@@ +284,5 @@
>      MOZ_ASSERT(memcmp(&mRec->mHash, ptr, sizeof(SHA1Sum::Hash)) == 0); ptr += sizeof(SHA1Sum::Hash);
>      mRec->mFrecency = NetworkEndian::readUint32(ptr); ptr += sizeof(uint32_t);
>      mRec->mOriginAttrsHash = NetworkEndian::readUint64(ptr); ptr += sizeof(uint64_t);
>      mRec->mExpirationTime = NetworkEndian::readUint32(ptr); ptr += sizeof(uint32_t);
> +    mRec->mOnStartTime = NetworkEndian::readInt32(ptr); ptr += sizeof(uint16_t);

readUint16

Comment 10

[Junior [inactive]]

(In reply to Michal Novotny (:michal) from comment #8)
> I agree that for racing decision we don't need to keep times longer than
> 65535ms, but this can affect HTTP_NET_VS_CACHE_ probes a lot. I think we can
> store uint32_t in the index. Or we could keep full time in metadata which
> would be used for telemetry and store uint16_t in the index which would be
> used only for racing decision.

To save memory usage, I'd like to use the latter solution: store uint16 to
index for RCWN and uint64 to cache file for telemetry.

Comment 11

[Junior [inactive]]

Attachment: Part1: add onstart/stop to cache index - v3

Change the cache storing policy addressed in last comment

Comment 13

[Junior [inactive]]

Attachment: Part2: expose to cache entry - v3

Change the cache storing policy addressed in comment 10

Comment 14

[Michal Novotny [:michal]]

Comment on attachment 8851871 [details] [diff] [review]
Part1: add onstart/stop to cache index - v3

Review of attachment 8851871 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFile.cpp
@@ +2397,5 @@
> +  static auto toUint16 = [](const char* s) -> uint16_t {
> +    if (s) {
> +      uint64_t n64 = atoi(s);
> +      return n64 <= UINT16_MAX ? n64 : UINT16_MAX;
> +    }

AFAIK atoi won't parse uint64_t, something like this would be probably better:

http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/dom/media/MediaManager.cpp#3134

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +3584,3 @@
>  {
>    LOG(("CacheFileIOManager::UpdateIndexEntry() [handle=%p, frecency=%s, "
> +       "expirationTime=%s onStartTime=%s, onStopTime=%s, hasAltData=%s]", aHandle,

hasAltData should be before onStartTime

::: netwerk/cache2/CacheIndex.cpp
@@ +2712,5 @@
>    }
>    aEntry->SetHasAltData(hasAltData);
>  
> +  const char *onStartTimeStr = aMetaData->GetElement("net-response-time-onstart");
> +  uint64_t onStartTime = onStartTimeStr ? (uint64_t) atoi(onStartTimeStr) : 0;

The same as in CacheFile::InitIndexEntry.

::: netwerk/cache2/CacheIndex.h
@@ +68,5 @@
>    uint32_t        mFrecency;
>    OriginAttrsHash mOriginAttrsHash;
>    uint32_t        mExpirationTime;
> +  uint16_t         mOnStartTime;
> +  uint16_t         mOnStopTime;

Please fix the indentation.

@@ +387,5 @@
>  
>    void InitNew()
>    {
>      mUpdateFlags = kFrecencyUpdatedMask | kExpirationUpdatedMask |
> +                   kHasAltDataUpdatedMask| kOnStartTimeUpdatedMask |

nit: put space between kHasAltDataUpdatedMask and the operator.

@@ +410,5 @@
>      mUpdateFlags |= kHasAltDataUpdatedMask;
>      CacheIndexEntry::SetHasAltData(aHasAltData);
>    }
>  
> +  void SetOnStartTime(int32_t aTime)

Use the same type of the argument as in CacheIndexEntry, i.e. uint16_t.

Comment 15

[Junior [inactive]]

Attachment: Part1: add onstart/stop to cache index - v4

Comment 16

[Michal Novotny [:michal]]

Comment on attachment 8851873 [details] [diff] [review]
Part2: expose to cache entry - v3

Review of attachment 8851873 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheFile.cpp
@@ +1234,5 @@
> +nsresult CacheFile::GetOnStartTime(uint64_t *_retval)
> +{
> +  MOZ_ASSERT(mMetadata);
> +  const char *onStartTimeStr = mMetadata->GetElement("net-response-time-onstart");
> +  *_retval = onStartTimeStr ? (uint64_t) atoi(onStartTimeStr) : 0;

When there is no such metadata, it should throw NS_ERROR_NOT_AVAILABLE instead of returning 0. Also please replace atoi() as in the other patch.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +6912,5 @@
>              !mCustomConditionalRequest &&
>              !mAsyncOpenTime.IsNull() && !mOnStartRequestTimestamp.IsNull()) {
> +            uint64_t onStartTime = (mOnStartRequestTimestamp - mAsyncOpenTime).ToMilliseconds();
> +            uint64_t onStopTime = (TimeStamp::Now() - mAsyncOpenTime).ToMilliseconds();
> +            Unused << mCacheEntry->SetNetworkTimes(onStartTime, onStopTime);

Could you also replace http://searchfox.org/mozilla-central/rev/7419b368156a6efa24777b21b0e5706be89a9c2f/netwerk/protocol/http/nsHttpChannel.cpp#8600 with calls to  CacheEntry::GetOn*Time?

Comment 17

[Michal Novotny [:michal]]

Comment on attachment 8852370 [details] [diff] [review]
Part1: add onstart/stop to cache index - v4

Review of attachment 8852370 [details] [diff] [review]:
-----------------------------------------------------------------

::: netwerk/cache2/CacheIndex.cpp
@@ +2724,5 @@
> +  };
> +
> +  aEntry->SetOnStartTime(getUint16MetaData("net-response-time-onstart"));
> +  aEntry->SetOnStopTime(getUint16MetaData("net-response-time-onstop"));
> +

This seems to be wrong. If there are no times in the metadata we set 0 in the index, which would be interpreted as very fast network response. We probably need a special value 0xFFFF that would mean that the time is not available.

Comment 18

[Junior [inactive]]

CacheIndex is always with a default value, but CacheFile and Metadata Element and not.

Therefore, the plan is
1) For cache index, 
  0xFFFF is for a not-available time
  0xFFFE is for those time greater than upper-bound

2) For cache file/entry
  Not-available time will throw NS_ERROR_NOT_AVAILABLE
  Since it's uint64_t, it has enough space to store the time.

Comment 19

[Junior [inactive]]

Attachment: Part1: add onstart/stop to cache index - v5

Comment 20

[Junior [inactive]]

Attachment: Part2: expose to cache entry - v4

Comment 21

[Junior [inactive]]

Thanks for your prompt and informative review, Michal!

Comment 22

[Junior [inactive]]

Attachment: Part1: add onstart/stop to cache index - v6

Fix nit and rebase, carry r+

Comment 23

[Junior [inactive]]

Attachment: Part2: expose to cache entry - v5

Fix bustage, add comments and rebase, carry r+

Comment 24

[Junior [inactive]]

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

Comment 25

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/913b6b05d4c1
Part 1: Add net-response-time-onstart/onstop to cache index. r=michal
https://hg.mozilla.org/integration/mozilla-inbound/rev/e7af85a4fd7b
Part 2: Expose the index-update to cache entry. r=michal

Comment 26

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/913b6b05d4c1
https://hg.mozilla.org/mozilla-central/rev/e7af85a4fd7b