Add net-response-time-onstart/onstop 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

(2 attachments, 9 obsolete attachments)

29.41 KB, patch
junior
: review+
Details | Diff | Splinter Review
10.75 KB, patch
junior
: review+
Details | Diff | Splinter Review
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.
Blocks: 1325341
Assignee: nobody → michal.novotny
Whiteboard: [necko-active]
steal assignee from michal
Assignee: michal.novotny → juhsu
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?
Flags: needinfo?(michal.novotny)
(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.
Flags: needinfo?(michal.novotny)
Posted patch nettime - WIP1 (obsolete) — Splinter Review
refactor for adding onstart/onstop parameters, and fixed some alt-data
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?
Flags: needinfo?(michal.novotny)
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.
Attachment #8850485 - Attachment is obsolete: true
Flags: needinfo?(michal.novotny)
Attachment #8851530 - Flags: review?(michal.novotny)
I keep the uint64_t in HttpChannel::ReportNetVSCacheTelemetry for backward-compat.
Attachment #8851531 - Flags: review?(michal.novotny)
(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 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
Attachment #8851530 - Flags: review?(michal.novotny) → feedback+
(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.
Change the cache storing policy addressed in last comment
Attachment #8851530 - Attachment is obsolete: true
Attachment #8851871 - Flags: review?(michal.novotny)
Change the cache storing policy addressed in comment 10
Attachment #8851872 - Attachment is obsolete: true
Attachment #8851872 - Flags: review?(michal.novotny)
Attachment #8851873 - Flags: review?(michal.novotny)
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.
Attachment #8851871 - Flags: review?(michal.novotny) → feedback+
Attachment #8851871 - Attachment is obsolete: true
Attachment #8852370 - Flags: review?(michal.novotny)
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?
Attachment #8851873 - Flags: review?(michal.novotny) → feedback+
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.
Attachment #8852370 - Flags: review?(michal.novotny) → feedback+
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.
Attachment #8852370 - Attachment is obsolete: true
Attachment #8852447 - Flags: review?(michal.novotny)
Attachment #8851873 - Attachment is obsolete: true
Attachment #8852448 - Flags: review?(michal.novotny)
Attachment #8852448 - Flags: review?(michal.novotny) → review+
Attachment #8852447 - Flags: review?(michal.novotny) → review+
Thanks for your prompt and informative review, Michal!
Fix nit and rebase, carry r+
Attachment #8852447 - Attachment is obsolete: true
Attachment #8852828 - Flags: review+
Fix bustage, add comments and rebase, carry r+
Attachment #8852448 - Attachment is obsolete: true
Attachment #8852829 - Flags: review+
Keywords: checkin-needed
Pushed by ryanvm@gmail.com:
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
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/913b6b05d4c1
https://hg.mozilla.org/mozilla-central/rev/e7af85a4fd7b
Status: NEW → RESOLVED
Closed: 2 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
Depends on: 1381766
You need to log in before you can comment on or make changes to this bug.