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

RESOLVED FIXED in Firefox 55

Status

()

Core
Networking: Cache
RESOLVED FIXED
a year ago
9 months 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)

(Reporter)

Description

a year ago
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.
(Reporter)

Updated

a year ago
Blocks: 1325341
(Reporter)

Updated

a year ago
Assignee: nobody → michal.novotny
Whiteboard: [necko-active]
(Assignee)

Comment 1

a year ago
steal assignee from michal
Assignee: michal.novotny → juhsu
(Assignee)

Comment 2

a year ago
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)
(Reporter)

Comment 3

a year ago
(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)
(Assignee)

Comment 4

a year ago
Created attachment 8850485 [details] [diff] [review]
nettime - WIP1

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

Comment 5

a year ago
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)
(Assignee)

Comment 6

a year ago
Created attachment 8851530 [details] [diff] [review]
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.
Attachment #8850485 - Attachment is obsolete: true
Flags: needinfo?(michal.novotny)
Attachment #8851530 - Flags: review?(michal.novotny)
(Assignee)

Comment 7

a year ago
Created attachment 8851531 [details] [diff] [review]
Part2: expose_to_cache_entry - v1

I keep the uint64_t in HttpChannel::ReportNetVSCacheTelemetry for backward-compat.
Attachment #8851531 - Flags: review?(michal.novotny)
(Reporter)

Comment 8

a year ago
(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.
(Reporter)

Comment 9

a year ago
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+
(Assignee)

Comment 10

a year ago
(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.
(Assignee)

Comment 11

a year ago
Created attachment 8851871 [details] [diff] [review]
Part1: add onstart/stop to cache index - v3

Change the cache storing policy addressed in last comment
Attachment #8851530 - Attachment is obsolete: true
Attachment #8851871 - Flags: review?(michal.novotny)
Comment hidden (spam)
(Assignee)

Comment 13

a year ago
Created attachment 8851873 [details] [diff] [review]
Part2: expose to cache entry - v3

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

Comment 14

a year ago
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+
(Assignee)

Comment 15

a year ago
Created attachment 8852370 [details] [diff] [review]
Part1: add onstart/stop to cache index - v4
Attachment #8851871 - Attachment is obsolete: true
Attachment #8852370 - Flags: review?(michal.novotny)
(Reporter)

Comment 16

a year ago
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+
(Reporter)

Comment 17

a year ago
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+
(Assignee)

Comment 18

a year ago
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.
(Assignee)

Comment 19

a year ago
Created attachment 8852447 [details] [diff] [review]
Part1: add onstart/stop to cache index - v5
Attachment #8852370 - Attachment is obsolete: true
Attachment #8852447 - Flags: review?(michal.novotny)
(Assignee)

Comment 20

a year ago
Created attachment 8852448 [details] [diff] [review]
Part2: expose to cache entry - v4
Attachment #8851873 - Attachment is obsolete: true
Attachment #8852448 - Flags: review?(michal.novotny)
(Reporter)

Updated

a year ago
Attachment #8852448 - Flags: review?(michal.novotny) → review+
(Reporter)

Updated

a year ago
Attachment #8852447 - Flags: review?(michal.novotny) → review+
(Assignee)

Comment 21

a year ago
Thanks for your prompt and informative review, Michal!
(Assignee)

Comment 22

a year ago
Created attachment 8852828 [details] [diff] [review]
Part1: add onstart/stop to cache index - v6

Fix nit and rebase, carry r+
Attachment #8852447 - Attachment is obsolete: true
Attachment #8852828 - Flags: review+
(Assignee)

Comment 23

a year ago
Created attachment 8852829 [details] [diff] [review]
Part2: expose to cache entry - v5

Fix bustage, add comments and rebase, carry r+
Attachment #8852448 - Attachment is obsolete: true
Attachment #8852829 - Flags: review+
(Assignee)

Updated

a year ago
Keywords: checkin-needed

Comment 25

a year ago
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

Comment 26

a year ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/913b6b05d4c1
https://hg.mozilla.org/mozilla-central/rev/e7af85a4fd7b
Status: NEW → RESOLVED
Last Resolved: a year ago
status-firefox55: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla55
status-firefox53: affected → ---
(Reporter)

Updated

9 months ago
Depends on: 1381766
You need to log in before you can comment on or make changes to this bug.