1128339 - Add telemetry probes to find out impact of bug 1120945

Status: RESOLVED FIXED

(Core :: Networking: Cache, defect)

Comment 1

[Dragana Damjanovic [:dragana]]

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).

Comment 2

[Michal Novotny [:michal]]

Attachment: patch v1

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)

Comment 3

[Honza Bambas (:mayhemer)]

(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.

Comment 4

[Honza Bambas (:mayhemer)]

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.

Comment 5

[Dragana Damjanovic [:dragana]]

(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.

Comment 6

[Honza Bambas (:mayhemer)]

(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.

Comment 7

[Dragana Damjanovic [:dragana]]

(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.

Comment 8

[Michal Novotny [:michal]]

Attachment: patch v2 - addressed reviewer's comments

Comment 9

[Jason Duell]

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?

Comment 10

[Michal Novotny [:michal]]

(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.

Comment 11

[Michal Novotny [:michal]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/2a21df64de7d

Comment 12

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/2a21df64de7d