Precache bytes cap should be based on total bytes fetched from network

Precache bytes cap should be based on total bytes fetched from network

The current behavior is that precache task will stop if size of a
resource exceeds max_bytes_per_resource or if total size of all
resources exceeds max_bytes_total.

But there are resources which are sent gzipped, and they are stored in
cache in gzipped. So the max bytes cap should check the bytes used in
network instead of the resource size.

BUG=625270

[Raj, 2016-07-01 20:41:08]

rajendrant@chromium.org changed reviewers:
+ twifkak@chromium.org

[Raj, 2016-07-01 20:41:08]

ptal

[twifkak, 2016-07-01 23:59:25]

Sorry I wasn't able to offer any solutions, only problems!

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:238:
source->GetTotalReceivedBytes() > max_bytes_) {
AFAICT this is only available after the request is finished, not during
download, so it won't let us cancel mid-stream:
https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=...

This is a problem since we're downloading from third-party servers and can't
trust that they won't send us a really big file.

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:238:
source->GetTotalReceivedBytes() > max_bytes_) {
Also, the proto says "This max applies only to new downloads; cached resources
are not capped." Is that still true? I don't have a strong opinion on what it
should be -- only that the documentation should agree.

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:449: if
(unfinished_work_->network_bytes() >
Does this include the size of cache resources, too? It should, as our goal with
this cap is to keep from eating the whole cache (doing so incrementally isn't
much better). Per the proto, "this includes new downloads as well as cached
resources."

[Raj, 2016-07-14 18:41:30]

I was not able to find a simple fix for this issue. I am willing to close this
CL and the issue. WDYT.

The open issue is that OnURLFetchDownloadProgress() could cancel gzipped
resources which are large than max_bytes_per_resource cap in uncompressed state.
But the network usage and cache usage of the resource will only be its
compressed size.

I checked that compressed resource sizes are accounted for in other places for
200 and 304 responses. Again, there could be an improvement for 304 response
where it uses few network bytes for validating, and that is not taken into
account.

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
File components/precache/core/precache_fetcher.cc (right):

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:238:
source->GetTotalReceivedBytes() > max_bytes_) {
On 2016/07/01 23:59:25, twifkak wrote:
> Also, the proto says "This max applies only to new downloads; cached resources
> are not capped." Is that still true? I don't have a strong opinion on what it
> should be -- only that the documentation should agree.

The proto is correct. Only new downloads have FetchStage::NETWORK and
max_bytes_per_resource cap is applied only for those.

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:238:
source->GetTotalReceivedBytes() > max_bytes_) {
On 2016/07/01 23:59:25, twifkak wrote:
> AFAICT this is only available after the request is finished, not during
> download, so it won't let us cancel mid-stream:
>
https://cs.chromium.org/chromium/src/net/url_request/url_fetcher_core.cc?rcl=...
> 
> This is a problem since we're downloading from third-party servers and can't
> trust that they won't send us a really big file.

Yes. You are right. This is updated only after request is finished. It does not
seem easy to implement this in net/ as well to return network bytes or
compressed size progressively.

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:276: response_bytes_ =
source->GetReceivedResponseContentLength();
I checked that response_bytes_ here includes bytes from network (for 200 OK) or
bytes from cache (for 304 Not-Modified). Also for gzipped resources the
compressed size is returned. So our calculation is correct.

https://codereview.chromium.org/2115043002/diff/1/components/precache/core/pr...
components/precache/core/precache_fetcher.cc:449: if
(unfinished_work_->network_bytes() >
On 2016/07/01 23:59:25, twifkak wrote:
> Does this include the size of cache resources, too? It should, as our goal
with
> this cap is to keep from eating the whole cache (doing so incrementally isn't
> much better). Per the proto, "this includes new downloads as well as cached
> resources."

My code is wrong. It should not be changed. total_bytes() accounts for bytes in
the network or bytes in the cache, depending on http response 200 or 304.

[twifkak, 2016-07-18 03:59:44]

On 2016/07/14 18:41:30, Raj wrote:
> I was not able to find a simple fix for this issue. I am willing to close this
> CL and the issue. WDYT.
> 
> The open issue is that OnURLFetchDownloadProgress() could cancel gzipped
> resources which are large than max_bytes_per_resource cap in uncompressed
state.
> But the network usage and cache usage of the resource will only be its
> compressed size.

Perhaps we could use uncompressed bytes for the per-resource cap while using
compressed bytes for the overall cap. But I'm also willing to close the issue --
doesn't seem critical to me -- but I could be wrong?

[Raj, 2016-07-18 18:17:52]

On 2016/07/18 03:59:44, twifkak wrote:
> On 2016/07/14 18:41:30, Raj wrote:
> > I was not able to find a simple fix for this issue. I am willing to close
this
> > CL and the issue. WDYT.
> > 
> > The open issue is that OnURLFetchDownloadProgress() could cancel gzipped
> > resources which are large than max_bytes_per_resource cap in uncompressed
> state.
> > But the network usage and cache usage of the resource will only be its
> > compressed size.
> 
> Perhaps we could use uncompressed bytes for the per-resource cap while using
> compressed bytes for the overall cap. But I'm also willing to close the issue
--
> doesn't seem critical to me -- but I could be wrong?

Yes. In succinct that is what we are doing.
OnURLFetchDownloadProgress() uses uncompressed bytes for the per-resource cap.
In all other places compressed bytes are accounted for.

[twifkak, 2016-07-19 00:18:23]

On 2016/07/18 18:17:52, Raj wrote:
> On 2016/07/18 03:59:44, twifkak wrote:
> > On 2016/07/14 18:41:30, Raj wrote:
> > > I was not able to find a simple fix for this issue. I am willing to close
> this
> > > CL and the issue. WDYT.
> > > 
> > > The open issue is that OnURLFetchDownloadProgress() could cancel gzipped
> > > resources which are large than max_bytes_per_resource cap in uncompressed
> > state.
> > > But the network usage and cache usage of the resource will only be its
> > > compressed size.
> > 
> > Perhaps we could use uncompressed bytes for the per-resource cap while using
> > compressed bytes for the overall cap. But I'm also willing to close the
issue
> --
> > doesn't seem critical to me -- but I could be wrong?
> 
> Yes. In succinct that is what we are doing.
> OnURLFetchDownloadProgress() uses uncompressed bytes for the per-resource cap.
> In all other places compressed bytes are accounted for.

Oh, okay, closing SGTM then. I think for the most part the compression ratio is
>50% (here are some examples:
https://developers.google.com/web/fundamentals/performance/optimizing-content...),
so we may be overeager in cancelling downloads, but not too much so.