1354405 - Add new telemetry probe for bandwidth cost based on average cache hit when we rcwn

Status: RESOLVED FIXED

(Core :: Networking: Cache, enhancement)

Description

[Junior [inactive]]

To evaluate rcwn algorithm, we need new probe for real bandwidth cost will be just an estimate based on average cache hit

Comment 1

[Junior [inactive]]

Tend to report from nsHttpTransaction::GetTransferSize

Comment 2

[Junior [inactive]]

We will get 3 probes:
1) not race/race 
2) use cache/race 
3) use network

+-----------------+---------------------+----------------------+----------------------+
|                 |       Not Race      | Race and Use Network |  Race and Use Cache  |
+-----------------+---------------------+----------------------+----------------------+
| No Entry        |         Net         |          Net         |          Net         |
+-----------------+---------------------+----------------------+----------------------+
| Stale Entry     |         Net         |          Net         |          Net         |
+-----------------+---------------------+----------------------+----------------------+
| Re-validate 304 | Cache and small net |          Net         |        Depends       |
+-----------------+---------------------+----------------------+----------------------+
| Re-validate 200 |         Net         |          Net         |          Net         |
+-----------------+---------------------+----------------------+----------------------+
| Fresh Entry     |        Cache        |          Net         |        Cache*        |
+-----------------+---------------------+----------------------+----------------------+

* After we enable cancel-net-on-cache-win

Comment 3

[Junior [inactive]]

Attachment: bandwidth - v1

The patch is based on Bug 1354409.

Comment 4

[Michal Novotny [:michal]]

Comment on attachment 8861343 [details] [diff] [review]
bandwidth - v1

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

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +7012,5 @@
>  
> +    if (mTransaction) {
> +        mTransferSize = mTransaction->GetTransferSize();
> +    }
> +    Telemetry::Accumulate(kRcwnTelemetry[rcwnStatus], mTransferSize);

See https://bugzilla.mozilla.org/show_bug.cgi?id=1354409#c11, the same apply for this probe.

::: toolkit/components/telemetry/Histograms.json
@@ +2165,5 @@
>    },
> +  "NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK": {
> +    "expires_in_version": "58",
> +    "kind": "exponential",
> +    "high": 1048576,

1MB upper limit looks quite small to me. Why did you choose this value?

@@ +2167,5 @@
> +    "expires_in_version": "58",
> +    "kind": "exponential",
> +    "high": 1048576,
> +    "n_buckets": 100,
> +    "description": "Amount of bytes sent when we decide to race cache with network and network wins.",

I think it should be "bytes received".

Comment 5

[Junior [inactive]]

> ::: toolkit/components/telemetry/Histograms.json
> @@ +2165,5 @@
> >    },
> > +  "NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK": {
> > +    "expires_in_version": "58",
> > +    "kind": "exponential",
> > +    "high": 1048576,
> 
> 1MB upper limit looks quite small to me. Why did you choose this value?
> 

I'd like to choose 1GB before. Thanks for pointing this.

Comment 6

[Junior [inactive]]

Attachment: bandwidth - v2

Comment 7

[Michal Novotny [:michal]]

Comment on attachment 8864018 [details] [diff] [review]
bandwidth - v2

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

::: toolkit/components/telemetry/Histograms.json
@@ +2076,5 @@
>    },
> +  "NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK": {
> +    "expires_in_version": "58",
> +    "kind": "exponential",
> +    "high": 1073741824,

I think this needs some tweaking. mTransferSize contains size including headers and it won't be usually smaller than few hundreds of bytes. So first 20-25 buckets would be unused and we would lose the most fine grained buckets. We need to have some reasonable low limit. And maybe with 1GB we should also have more than 100 buckets to have a good resolution for sizes <= 1MB.

Comment 8

[Junior [inactive]]

(In reply to Michal Novotny (:michal) from comment #7)
> Comment on attachment 8864018 [details] [diff] [review]
> bandwidth - v2
> 
> Review of attachment 8864018 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: toolkit/components/telemetry/Histograms.json
> @@ +2076,5 @@
> >    },
> > +  "NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK": {
> > +    "expires_in_version": "58",
> > +    "kind": "exponential",
> > +    "high": 1073741824,
> 
> I think this needs some tweaking. mTransferSize contains size including
> headers and it won't be usually smaller than few hundreds of bytes. So first
> 20-25 buckets would be unused and we would lose the most fine grained
> buckets. We need to have some reasonable low limit. 

I'll grab some statistics for the header size.
However, H2 might have extremely small header size since HPACK compression, frequently < 100bytes.
Gathering <32 bytes into one bucket might be good.

> And maybe with 1GB we
> should also have more than 100 buckets to have a good resolution for sizes
> <= 1MB.

for 100 buckets, the rate is around 1.23
for 300 buckets, the rate is around 1.07, sounds better.

Comment 9

[Junior [inactive]]

1) Per SPDY_SYN_REPLY_SIZE telemetry, 4% of H2 header size is <16 bytes.
I don't think it's a good idea to do much tweaking.

2) Without special approval, we can not have more than 100 buckets.

3) We won't race if the size of content is expected to be big.

Based on the above facts, I choose low=32 and high=16MB
The rate would be 1.14, which is similar to SPDY_SYN_REPLY_SIZE

Comment 10

[Junior [inactive]]

Attachment: bandwidth - v3

Modify by last comment, also do some refactor.

Comment 11

[Michal Novotny [:michal]]

Comment on attachment 8864482 [details] [diff] [review]
bandwidth - v3

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

r+ but please change the expiration and also make sure to ask for data review before landing

::: toolkit/components/telemetry/Histograms.json
@@ +2083,5 @@
>      "n_buckets": 100,
>      "description": "Time in milliseconds between onStartRequest from the cache and onCacheEntryAvailable. Report only when net wins and OCEC is before onStartRequest from net."
>    },
> +  "NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK_WIN": {
> +    "expires_in_version": "58",

I think we want to keep these probes longer than till release 58.

Comment 12

[Junior [inactive]]

Attachment: bandwidth -v4

Data review for additive telemetries.

These telemetries record the bandwidth of different conditions when we race cache with network, in order to measure the trade-off network data from earning speed.

Comment 13

[Junior [inactive]]

https://treeherder.mozilla.org/#/jobs?repo=try&revision=1f6ec8d6865fc11376533c25ce80669dc048123d

Comment 14

[Benjamin Smedberg]

Comment on attachment 8864807 [details] [diff] [review]
bandwidth -v4

In addition to the necko@m.c mailing list, please add a real person's name to the alert_emails so that we know who's responsible for this probe.

data-r=me with that change

Comment 15

[Junior [inactive]]

Attachment: bandwidth - v5

Rebase on bug 1363700.
Good to have another eye on this small patch again.

Comment 16

[Valentin Gosu [:valentin] (he/him)]

Comment on attachment 8868492 [details] [diff] [review]
bandwidth - v5

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

r+ with this one question answered.

::: netwerk/protocol/http/nsHttpChannel.cpp
@@ +8855,5 @@
> +        Telemetry::NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK_WIN,
> +        Telemetry::NETWORK_RACE_CACHE_BANDWITH_RACE_CACHE_WIN,
> +        Telemetry::NETWORK_RACE_CACHE_BANDWITH_NOT_RACE,
> +        Telemetry::NETWORK_RACE_CACHE_BANDWITH_NOT_RACE
> +    };

Isn't this the other way around? Shouldn't kDidNotRaceUsedNetwork go into the NETWORK_RACE_CACHE_BANDWITH_NOT_RACE bucket, and kRaceUsedCache go into NETWORK_RACE_CACHE_BANDWITH_RACE_CACHE_WIN ?

Comment 17

[Michal Novotny [:michal]]

Good catch, looks like following order is correct:

Telemetry::NETWORK_RACE_CACHE_BANDWITH_NOT_RACE,
Telemetry::NETWORK_RACE_CACHE_BANDWITH_NOT_RACE
Telemetry::NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK_WIN,
Telemetry::NETWORK_RACE_CACHE_BANDWITH_RACE_CACHE_WIN,

Comment 18

[Junior [inactive]]

> Isn't this the other way around? Shouldn't kDidNotRaceUsedNetwork go into
> the NETWORK_RACE_CACHE_BANDWITH_NOT_RACE bucket, and kRaceUsedCache go into
> NETWORK_RACE_CACHE_BANDWITH_RACE_CACHE_WIN ?
>
> Good catch, looks like following order is correct:
> 
> Telemetry::NETWORK_RACE_CACHE_BANDWITH_NOT_RACE,
> Telemetry::NETWORK_RACE_CACHE_BANDWITH_NOT_RACE
> Telemetry::NETWORK_RACE_CACHE_BANDWITH_RACE_NETWORK_WIN,
> Telemetry::NETWORK_RACE_CACHE_BANDWITH_RACE_CACHE_WIN,

Yes, it's correct. Thanks for saving this horrible mistake.

Comment 19

[Junior [inactive]]

Attachment: bandwidth -v6

Comment 20

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/bb52b557bc32
Add new telemetry probe for bandwidth cost for rcwn, r=michal, valentin, data-r=bsmedberg

Comment 21

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/bb52b557bc32