687075 - Add telemetry for lifetime of disk cache entry

Status: NEW

(Core :: Networking: Cache, defect, P5)

Description

[u408661]

Initially from the last modified timestamp to the time of eviction, eventually we would like to have it from the initial fetch time.

Comment 1

[u408661]

Attachment: patch

Not sure how I feel about this, even though it's really the only way to get the statistic we want here. Are we OK with increasing the amount of disk I/O to get this number? It's probably not as big an issue on mobile (since the real I/O bottleneck on mobile is writing), but still worth thinking about.

Comment 2

[Jason Duell]

Comment on attachment 561781 [details] [diff] [review]
patch

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

::: netwerk/cache/nsDiskCacheDevice.cpp
@@ +188,5 @@
>          // entry not in use, just delete storage because we're enumerating the records
> +        nsDiskCacheEntry * diskEntry = mCacheMap->ReadDiskCacheEntry(mapRecord);
> +        if (diskEntry)
> +            Telemetry::Accumulate(Telemetry::DISK_CACHE_ENTRY_LIFETIME,
> +                SecondsFromPRTime(PR_Now()) - diskEntry->mLastModified);

I assume this is on an async thread, so the extra I/O isn't necessarily a problem, per se.   But we should make sure before we land something like this.  But how to measure?  Meh.  It's tricky--hard to measure the overall system perf hit from doing a little more I/O.

Michal, thoughts?

Comment 3

[Jason Duell]

Taras, who are these I/O experts who've recently joined us?

Comment 4

[(dormant account)]

(In reply to Jason Duell (:jduell) from comment #3)
> Taras, who are these I/O experts who've recently joined us?

David Teller is one, Vlad is starting in 2 weeks.

Why aren't you getting the last-modified time from the filesystem?

Comment 5

[u408661]

(In reply to Taras Glek (:taras) from comment #4)
> Why aren't you getting the last-modified time from the filesystem?

Because the cache data may be in a block file where the last-modified time in the fs may or may not apply to the entry we care about.

Comment 6

[(dormant account)]

(In reply to Nick Hurley from comment #5)
> (In reply to Taras Glek (:taras) from comment #4)
> > Why aren't you getting the last-modified time from the filesystem?
> 
> Because the cache data may be in a block file where the last-modified time
> in the fs may or may not apply to the entry we care about.

Might make sense to special-case that to cut overhead.

Comment 7

[u408661]

(In reply to Taras Glek (:taras) from comment #6)
> Might make sense to special-case that to cut overhead.

Originally I thought it might, but then I remembered some of the other cache telemetry bugs I filed (specifically bug 687077 and bug 687078). For those, we're going to have to do this exact same I/O at the exact same time. So I guess the question becomes, are these 3 pieces of data worth having the extra I/O for? (Not a question specifically for you, Taras, but one that would be nice to have your input on.)

Comment 8

[Patrick McManus [:mcmanus]]

you can always sample 1 in 16 or somesuch..

Comment 9

[u408661]

Not getting to this any time soon

Comment 10

[Firefox Bug Husbandry Bot]

Bulk change to priority: https://bugzilla.mozilla.org/show_bug.cgi?id=1399258