Closed Bug 1128339 Opened 5 years ago Closed 5 years ago

Add telemetry probes to find out impact of bug 1120945

Categories

(Core :: Networking: Cache, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: michal, Assigned: michal)

References

Details

Attachments

(1 file, 1 obsolete file)

No description provided.
Blocks: 1120945
I will write just some thought... 

When cache hits a limit (FAT32 limit or size limit), we are evicting items depending on the expiration time and the frequency.

We are interested in new request coming after the limit on FAT32 is filled. So it will be interesting to have information when a new coming request is found in cache on which position in frequency queue it was (if it is one of the candidates to be evicted). 
And some kind of histogram of expiration times of items in cache and a histogram of the frequencies of the items will be interesting (maybe we have this already).
Attached patch patch v1 (obsolete) — Splinter Review
The patch adds 2 telemetry probes
- type of FS that the cache is stored on (is reported only once)
- size of the cache when the file count limit on FAT32 is reached (is reported only once per session)
Attachment #8560490 - Flags: review?(honzab.moz)
(In reply to Dragana Damjanovic [:dragana] from comment #1)
> I will write just some thought... 
> 
> When cache hits a limit (FAT32 limit or size limit), we are evicting items
> depending on the expiration time and the frequency.

*Frecency*.  Yes.

> 
> We are interested in new request coming after the limit on FAT32 is filled.
> So it will be interesting to have information when a new coming request is
> found in cache on which position in frequency queue it was (if it is one of
> the candidates to be evicted). 

Not sure I understand...

> And some kind of histogram of expiration times of items in cache and a
> histogram of the frequencies of the items will be interesting (maybe we have
> this already).

We don't have that and it's hard to obtain.  Definitely no idea how to "histogramize" frecency.  And also, I don't see a point.
Depends on: 1130784
Comment on attachment 8560490 [details] [diff] [review]
patch v1

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

we first need to solve the error no problem.

::: netwerk/cache2/CacheFileIOManager.cpp
@@ +3661,5 @@
> +        // Report the full size only once per session
> +        static bool sSizeReported = false;
> +        if (!sSizeReported) {
> +          uint32_t cacheUsage;
> +          rv = CacheIndex::GetCacheSize(&cacheUsage);

you are rewriting rv of OpenNSPRFileDesc!

::: netwerk/cache2/CacheObserver.cpp
@@ +236,5 @@
>    mozilla::Preferences::AddBoolVarCache(
>      &sClearCacheOnShutdown, "privacy.clearOnShutdown.cache", kDefaultClearCacheOnShutdown);
> +
> +  mozilla::Preferences::AddBoolVarCache(
> +    &sCacheFSReported, "browser.cache.disk.filesystem_reported", kDefaultCacheFSReported);

isn't it enough for you to just read the value?  do you really need to observe changes?  the observer is resource wasting if not needed.
Attachment #8560490 - Flags: review?(honzab.moz)
(In reply to Honza Bambas (:mayhemer) from comment #3)

> > 
> > We are interested in new request coming after the limit on FAT32 is filled.
> > So it will be interesting to have information when a new coming request is
> > found in cache on which position in frequency queue it was (if it is one of
> > the candidates to be evicted). 
> 
> Not sure I understand...
> 

To estimate impact of the FAT32 limit we should use users without FAT32 and we should try to estimate in which extend the experience is worse when they would have the FAT32's limit.
For that we should observe the requests that are served from the cache when the limit is reach. If the frequency of a request served from cache is low (if the cache entry would be evict because of the limit) that would be a negative impact of the FAT32 limit.

> > And some kind of histogram of expiration times of items in cache and a
> > histogram of the frequencies of the items will be interesting (maybe we have
> > this already).
> 
> We don't have that and it's hard to obtain.  Definitely no idea how to
> "histogramize" frecency.  And also, I don't see a point.

This can be done. It can be use to estimate optimal size of a cache but it is not that important for this test.
(In reply to Dragana Damjanovic [:dragana] from comment #5)
> (In reply to Honza Bambas (:mayhemer) from comment #3)
> 
> > > 
> > > We are interested in new request coming after the limit on FAT32 is filled.
> > > So it will be interesting to have information when a new coming request is
> > > found in cache on which position in frequency queue it was (if it is one of
> > > the candidates to be evicted). 
> > 
> > Not sure I understand...
> > 
> 
> To estimate impact of the FAT32 limit we should use users without FAT32 and
> we should try to estimate in which extend the experience is worse when they
> would have the FAT32's limit.
> For that we should observe the requests that are served from the cache when
> the limit is reach. If the frequency of a request served from cache is low
> (if the cache entry would be evict because of the limit) that would be a
> negative impact of the FAT32 limit.

Still not sure I completely understand.  Do you mean to actually engage the 13106 files limit for normal users and have an experiment against normal cache settings users?

> 
> > > And some kind of histogram of expiration times of items in cache and a
> > > histogram of the frequencies of the items will be interesting (maybe we have
> > > this already).
> > 
> > We don't have that and it's hard to obtain.  Definitely no idea how to
> > "histogramize" frecency.  And also, I don't see a point.
> 
> This can be done. It can be use to estimate optimal size of a cache but it
> is not that important for this test.

If you can outline a concrete plan on what numbers you want to collect and how exactly you want to decide on those obtained numbers how to optimize the cache size (if only that) then please do.  Then we can all check on that and see if it's worth to do.  But definitely not in this bug, open a new one please.
(In reply to Honza Bambas (:mayhemer) from comment #6)
> (In reply to Dragana Damjanovic [:dragana] from comment #5)

> > 
> > To estimate impact of the FAT32 limit we should use users without FAT32 and
> > we should try to estimate in which extend the experience is worse when they
> > would have the FAT32's limit.
> > For that we should observe the requests that are served from the cache when
> > the limit is reach. If the frequency of a request served from cache is low
> > (if the cache entry would be evict because of the limit) that would be a
> > negative impact of the FAT32 limit.
> 
> Still not sure I completely understand.  Do you mean to actually engage the
> 13106 files limit for normal users and have an experiment against normal
> cache settings users?
> 

Not force the limit to the normal users, but just do statistics. Just send telemetry if a request that is served from cache has a so low frequency so that it is on 13106 + something place sorted by the frequency value and actually would not be there if we have had the limit. It is just a theoretical value.

This telemetry value will not be really accurate because the cache dynamics, with and without limit, are different but it will show some measurements.


> If you can outline a concrete plan on what numbers you want to collect and
> how exactly you want to decide on those obtained numbers how to optimize the
> cache size (if only that) then please do.  Then we can all check on that and
> see if it's worth to do.  But definitely not in this bug, open a new one
> please.


ok.
Attachment #8560490 - Attachment is obsolete: true
Attachment #8562203 - Flags: review?(honzab.moz)
Attachment #8562203 - Flags: review?(honzab.moz) → review?(jduell.mcbugs)
Comment on attachment 8562203 [details] [diff] [review]
patch v2 - addressed reviewer's comments

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

So we all seem to have different notions of what telemetry we need here :)  (I'm afraid I don't understand exactly what Dragana's metric would be--feel free to keep trying to explain it, Dragana).

So this patch is gathering the size of FAT32 caches when we hit the file count limit.  I don't see what that gets us, exactly.  It's not bad to know, but I was thinking that we'd just compare the overall cache hit rate for FAT32 users vs non-FAT32.  Isn't that the most obvious way to see if the file count limit affects cache performance?
(In reply to Jason Duell [:jduell] (needinfo? me) from comment #9)
> So this patch is gathering the size of FAT32 caches when we hit the file
> count limit.  I don't see what that gets us, exactly.  It's not bad to know,
> but I was thinking that we'd just compare the overall cache hit rate for
> FAT32 users vs non-FAT32.  Isn't that the most obvious way to see if the
> file count limit affects cache performance?

Telemetry in this patch give us information about how many people are affected and what's the average maximum cache size on FAT32. The hit rate for different cache sizes (in file count) give us telemetry from bug 1131600.
Attachment #8562203 - Flags: review?(jduell.mcbugs) → review+
https://hg.mozilla.org/mozilla-central/rev/2a21df64de7d
Status: NEW → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
You need to log in before you can comment on or make changes to this bug.