Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1506023002:
Minimize the number of range requests made by PDFium (Closed)
DescriptionMinimize the number of range requests made by PDFium
Previously, Chrome would make a series of range requests for linearized PDFs,
starting at 32 KB and slowly increasing. On high latency connections, this is
considerably slower than using a single request. Now Chrome will make range
requests as large as possible and cancel them if the renderer needs some data in
a different place in the document. This significantly reduces the number of
range requests performed by Chrome.
BUG=460201, 78264
Committed: https://crrev.com/3ba2a28104c4d6feef3efd71d9be73f085886f53
Cr-Commit-Position: refs/heads/master@{#364235}
Patch Set 1 #Patch Set 2 : DidOpen is never cancelled #
Total comments: 22
Patch Set 3 : Cleanup and don't partially render full documents #
Total comments: 21
Patch Set 4 : Cleanup #
Total comments: 2
Patch Set 5 : delint #Patch Set 6 : Fix issue with chunk_stream #
Messages
Total messages: 42 (14 generated)
The CQ bit was checked by spelchat@chromium.org
The CQ bit was unchecked by spelchat@chromium.org
great work-in-progress https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:219: // Remove all elements except the first one. code and comment disagree https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:279: // Start with size 0, we'll set extended_size to > 0. This way this request extended_size_ https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:280: // will get cancelled as soon as the renderer wants an other portion of the another https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:308: // Don't leave a gap smaller than kDefaultRequestSize It may be possible we can shift the gap management problem up to PDFium. It knows up front exactly which ranges are required to render each page. This could also potentially inform when is the best time to break off a connection. And it might make us less dependent on kDefaultRequestSize as a tuning number that might work better on some documents than others. That said, this code is short and sweet, does the job, and looks easy to modify when or if the time comes, and doesn't seem like it would get in the way. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:469: client_->OnPendingRequestComplete(); Same code is also on line 539. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:475: // If there are pending requests and the current content we're dowloading typo: dowloading https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:486: // Cancel the request is it's not satisfying any request from the *as* it's https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:497: } else if (result == PP_OK || result == PP_ERROR_ABORTED) { result cannot equal PP_ERROR_ABORTED here because the if clause above will have handled it. if (result > 0) { ... https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:507: // the end of the requested range. I might rewrite the comment: Returns true if we are already in progress satisfying the request, or just about ready to start. This helps us avoid expensive jumping around, and even worse leaving tiny gaps in the byte stream that might have to be filled later. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.h#n... pdf/document_loader.h:86: // Is the loader currently satisfying a the given request. typo: "a"
https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:219: // Remove all elements except the first one. On 2015/12/08 02:52:41, jab wrote: > code and comment disagree Done. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:279: // Start with size 0, we'll set extended_size to > 0. This way this request On 2015/12/08 02:52:41, jab wrote: > extended_size_ Done. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:280: // will get cancelled as soon as the renderer wants an other portion of the On 2015/12/08 02:52:41, jab wrote: > another Done. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:308: // Don't leave a gap smaller than kDefaultRequestSize On 2015/12/08 02:52:41, jab wrote: > It may be possible we can shift the gap management problem up to PDFium. It > knows up front exactly which ranges are required to render each page. This could > also potentially inform when is the best time to break off a connection. And it > might make us less dependent on kDefaultRequestSize as a tuning number that > might work better on some documents than others. > > That said, this code is short and sweet, does the job, and looks easy to modify > when or if the time comes, and doesn't seem like it would get in the way. Acknowledged. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:469: client_->OnPendingRequestComplete(); On 2015/12/08 02:52:41, jab wrote: > Same code is also on line 539. Thanks I forgot that copy paste. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:475: // If there are pending requests and the current content we're dowloading On 2015/12/08 02:52:41, jab wrote: > typo: dowloading Done. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:486: // Cancel the request is it's not satisfying any request from the On 2015/12/08 02:52:41, jab wrote: > *as* it's Done. https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:497: } else if (result == PP_OK || result == PP_ERROR_ABORTED) { On 2015/12/08 02:52:41, jab wrote: > result cannot equal PP_ERROR_ABORTED here because the if clause above will have > handled it. > > if (result > 0) { ... Are you sure? PP_ERROR_ABORTED = -3 https://code.google.com/p/chromium/codesearch#chromium/src/ppapi/c/pp_errors.... https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:507: // the end of the requested range. On 2015/12/08 02:52:41, jab wrote: > I might rewrite the comment: > > Returns true if we are already in progress satisfying the request, > or just about ready to start. This helps us avoid expensive > jumping around, and even worse leaving tiny gaps in the byte stream > that might have to be filled later. This is way better, thanks! https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.h#n... pdf/document_loader.h:86: // Is the loader currently satisfying a the given request. On 2015/12/08 02:52:41, jab wrote: > typo: "a" Done.
Looks good to me. I probably don't have enough authority to provide an official LGTM
breidenbach@gmail.com changed reviewers: + breidenbach@gmail.com
https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:497: } else if (result == PP_OK || result == PP_ERROR_ABORTED) { Oh, you are right. You know what fooled me? I saw the NOTREACHED() call below and thought to myself, execution should never reach there. But then what about all those other possible return values? Had they all been positive, then we'd get to NOTREACHED(). And then when I looked at the values I managed to not see the negative signs. Anyway, you are fine as written. I'm still a little concerned that all the other possible values are getting dumped into the NOTREACHED() clause, but that was existing behavior. Can be considered outside the scope of this changelist especially if the right thing to do is not obvious. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:17: #define kDefaultRequestSize 65536u There is a similar constant in the cc file, called const uint32_t kMinFileSize = 64 * 1024. Might make sense to put these two constants next to each other. Also, I would add a comment just in case someone is tempted to adjust this tuning number in the future. Something like this. // Number was chosen in crbug/460201#c37 #define kDefaultRequestSize 65536u
Description was changed from ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201 ========== to ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201 ==========
spelchat@chromium.org changed reviewers: + thestig@chromium.org - breidenbach@gmail.com
https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/20001/pdf/document_loader.cc#... pdf/document_loader.cc:497: } else if (result == PP_OK || result == PP_ERROR_ABORTED) { On 2015/12/08 18:32:23, jab wrote: > Oh, you are right. You know what fooled me? I saw the NOTREACHED() call below > and thought to myself, execution should never reach there. But then what about > all those other possible return values? Had they all been positive, then we'd > get to NOTREACHED(). And then when I looked at the values I managed to not see > the negative signs. > > Anyway, you are fine as written. I'm still a little concerned that all the other > possible values are getting dumped into the NOTREACHED() clause, but that was > existing behavior. Can be considered outside the scope of this changelist > especially if the right thing to do is not obvious. > Yes, asserting the errors away seems odd (some don't seem deterministic), but I'll leave that battle to someone who knows better about Chrome ;). https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:17: #define kDefaultRequestSize 65536u On 2015/12/08 18:32:23, jab wrote: > There is a similar constant in the cc file, called > const uint32_t kMinFileSize = 64 * 1024. Might make > sense to put these two constants next to each other. > > Also, I would add a comment just in case someone is > tempted to adjust this tuning number in the future. > Something like this. > > // Number was chosen in crbug/460201#c37 > #define kDefaultRequestSize 65536u kDefaultRequestSize to go in the h file, but kMinFileSize doesn't have to. I suppose leaving kMinFileSize in the cc file helps limiting evil macros. Maybe making kMinFileSize a static const and using the enum hack for kDefaultRequestSize would be the best (next to each other, in the h file without dangerous macros)? crbug/460201 is restricted, so I suppose we can't refer to it in the code (thestig@, can you confirm?).
On 2015/12/08 18:48:55, spelchat wrote: > crbug/460201 is restricted, so I suppose we can't refer to it in the code > (thestig@, can you confirm?). Is there anything confidential in there? If not, I'll unrestrict it.
Just nit picking. Are there any potential integer underflows in the calculations? https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:180: // 0 and ends at document_size_. Refer to variables as |foo|, e.g. |document_size_| here. Ditto below. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:298: pos = pos + size - kDefaultRequestSize; +=, also on line 302. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:474: if (pending_requests_.size() > 0) { !pending_requests_.empty() https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:480: for (auto pending_request : pending_requests_) { const auto & https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:504: // Returns true if we are already in progress satisfying the request, or just Documentation goes in the header. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:550: } // namespace chrome_pdf 2 spaces and a blank line above like before. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:87: bool SatisfyingRequest(size_t pos, size_t size); const method https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:104: size_t current_request_offset_; Can you describe these variables? https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:119: } // namespace chrome_pdf These were fine before.
On 2015/12/08 19:21:57, Lei Zhang wrote: > On 2015/12/08 18:48:55, spelchat wrote: > > crbug/460201 is restricted, so I suppose we can't refer to it in the code > > (thestig@, can you confirm?). > > Is there anything confidential in there? If not, I'll unrestrict it. #22, #26 and bits of #29 and #34 may be confidential. I could delete these comments (I wish I could just edit out the parts we don't want to disclose). I could just repost the bits of #26, #29 and #34 that are relevant to tuning kDefaultRequestSize in crbug.com/78264 which is not restricted.
On 2015/12/08 19:35:03, spelchat wrote: > On 2015/12/08 19:21:57, Lei Zhang wrote: > > On 2015/12/08 18:48:55, spelchat wrote: > > > crbug/460201 is restricted, so I suppose we can't refer to it in the code > > > (thestig@, can you confirm?). > > > > Is there anything confidential in there? If not, I'll unrestrict it. > > #22, #26 and bits of #29 and #34 may be confidential. I could delete these > comments (I wish I could just edit out the parts we don't want to disclose). I > could just repost the bits of #26, #29 and #34 that are relevant to tuning > kDefaultRequestSize in crbug.com/78264 which is not restricted. Ok, I won't unrestrict it then. Let's just leave it be. Deleted comments will just make the conversation awkward. If you want to use the similar bug 78264 as a public place to dump your findings, that sounds good to me.
I checked again for underflows, everything seems good. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:180: // 0 and ends at document_size_. On 2015/12/08 19:32:22, Lei Zhang wrote: > Refer to variables as |foo|, e.g. |document_size_| here. > > Ditto below. Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:298: pos = pos + size - kDefaultRequestSize; On 2015/12/08 19:32:22, Lei Zhang wrote: > +=, also on line 302. Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:474: if (pending_requests_.size() > 0) { On 2015/12/08 19:32:22, Lei Zhang wrote: > !pending_requests_.empty() Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:480: for (auto pending_request : pending_requests_) { On 2015/12/08 19:32:22, Lei Zhang wrote: > const auto & Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:504: // Returns true if we are already in progress satisfying the request, or just On 2015/12/08 19:32:22, Lei Zhang wrote: > Documentation goes in the header. Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.cc#... pdf/document_loader.cc:550: } // namespace chrome_pdf On 2015/12/08 19:32:22, Lei Zhang wrote: > 2 spaces and a blank line above like before. Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:17: #define kDefaultRequestSize 65536u On 2015/12/08 18:48:55, spelchat wrote: > On 2015/12/08 18:32:23, jab wrote: > > There is a similar constant in the cc file, called > > const uint32_t kMinFileSize = 64 * 1024. Might make > > sense to put these two constants next to each other. > > > > Also, I would add a comment just in case someone is > > tempted to adjust this tuning number in the future. > > Something like this. > > > > // Number was chosen in crbug/460201#c37 > > #define kDefaultRequestSize 65536u > > kDefaultRequestSize to go in the h file, but kMinFileSize doesn't have to. I > suppose leaving kMinFileSize in the cc file helps limiting evil macros. Maybe > making kMinFileSize a static const and using the enum hack for > kDefaultRequestSize would be the best (next to each other, in the h file without > dangerous macros)? > > crbug/460201 is restricted, so I suppose we can't refer to it in the code > (thestig@, can you confirm?). Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:87: bool SatisfyingRequest(size_t pos, size_t size); On 2015/12/08 19:32:22, Lei Zhang wrote: > const method Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:104: size_t current_request_offset_; On 2015/12/08 19:32:22, Lei Zhang wrote: > Can you describe these variables? Done. https://codereview.chromium.org/1506023002/diff/40001/pdf/document_loader.h#n... pdf/document_loader.h:119: } // namespace chrome_pdf On 2015/12/08 19:32:22, Lei Zhang wrote: > These were fine before. Done.
Description was changed from ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201 ========== to ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201,78264 ==========
lgtm I'm not familiar with the code, but if you two have been poking at it for a while and agree it's the way to go, then go for it. https://codereview.chromium.org/1506023002/diff/60001/pdf/document_loader.cc File pdf/document_loader.cc (right): https://codereview.chromium.org/1506023002/diff/60001/pdf/document_loader.cc#... pdf/document_loader.cc:280: else { goes on prev line https://codereview.chromium.org/1506023002/diff/60001/pdf/document_loader.h File pdf/document_loader.h (right): https://codereview.chromium.org/1506023002/diff/60001/pdf/document_loader.h#n... pdf/document_loader.h:131: } // namespace chrome_pdf Still needs 2 spaces. Ditto below.
re: nits - basically you should run "git cl lint" and not make me do it. :-P
On 2015/12/08 22:43:20, Lei Zhang wrote: > re: nits - basically you should run "git cl lint" and not make me do it. :-P Oops sorry, didn't know about git cl lint (I thought presubmit did something like that), I should have RTFM ;).
The CQ bit was checked by spelchat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from breidenbach@gmail.com, thestig@chromium.org Link to the patchset: https://codereview.chromium.org/1506023002/#ps80001 (title: "delint")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1506023002/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1506023002/80001
I patched this in, instrumented the range requests, hit reload a few times, fiddled with the scroll bar, trying to cause trouble. Managed to produce an infinite loop of nonsensical range requests. Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 Range: bytes=177669-177668 ...
1) Start loading PDF #1 2) Midway through load, switch to PDF #2. Expected: immediate switch to PDF #2 Actual: PDF #1 finishes loading, then we switch. (regression)
I can confirm that PDFium is not asking for the 0 size request. This is document_loader.cc; looks like an off-by-one error during gap filling. I'll try to find it tomorrow. ... Range: bytes=2049598-2115133 Range: bytes=414558-2049597 Range: bytes=1810920-2049597 Range: bytes=480094-1810919 Range: bytes=102176-2115133 Range: bytes=102125-2115133 Range: bytes=1810920-1810919 Range: bytes=1810920-1810919 Range: bytes=1810920-1810919 Range: bytes=1810920-1810919 Range: bytes=1810920-1810919 ...
The CQ bit was unchecked by spelchat@chromium.org
The CQ bit was unchecked by commit-bot@chromium.org
On 2015/12/09 01:50:55, jab wrote: > I can confirm that PDFium is not asking for the 0 size request. This is > document_loader.cc; looks > like an off-by-one error during gap filling. I'll try to find it tomorrow. > > ... > Range: bytes=2049598-2115133 > Range: bytes=414558-2049597 > Range: bytes=1810920-2049597 > Range: bytes=480094-1810919 > Range: bytes=102176-2115133 > Range: bytes=102125-2115133 > Range: bytes=1810920-1810919 > Range: bytes=1810920-1810919 > Range: bytes=1810920-1810919 > Range: bytes=1810920-1810919 > Range: bytes=1810920-1810919 > ... Didn't manage to reproduce either issue, but #23 and #25 happen because if (first_byte_after < pos + size - 1) should be if (first_byte_after < pos + size). I'll update the patch and go through the code again for off-by-one and underflow errors.
Patched in "if (first_byte_after < pos + size)". Took me about 25+ tries, but I was eventually able to produce a bad range request. I'll see if I can find it. Maybe should probably put a DCHECK(size > 0) as an additional precaution. Range: bytes=1834851-1834850 (0) Range: bytes=1834851-1834850 (0) Range: bytes=1834851-1834850 (0) Range: bytes=1834851-1834850 (0) ...
On 2015/12/09 17:07:09, spelchat wrote: > On 2015/12/09 01:50:55, jab wrote: > > I can confirm that PDFium is not asking for the 0 size request. This is > > document_loader.cc; looks > > like an off-by-one error during gap filling. I'll try to find it tomorrow. > > > > ... > > Range: bytes=2049598-2115133 > > Range: bytes=414558-2049597 > > Range: bytes=1810920-2049597 > > Range: bytes=480094-1810919 > > Range: bytes=102176-2115133 > > Range: bytes=102125-2115133 > > Range: bytes=1810920-1810919 > > Range: bytes=1810920-1810919 > > Range: bytes=1810920-1810919 > > Range: bytes=1810920-1810919 > > Range: bytes=1810920-1810919 > > ... > > Didn't manage to reproduce either issue, but #23 and #25 happen because if > (first_byte_after < pos + size - 1) should be if (first_byte_after < pos + > size). I'll update the patch and go through the code again for off-by-one and > underflow errors. Yes I noticed, GetFirstByteAfter(x) can return values <= x when there was no data past x (it returned the first missing byte of the missing bytes interval is part of). I changed it, so it should now be simpler. Also GetLastByteBefore had a name that didn't describe what it was actually doing. I used http://scholar.princeton.edu/sites/default/files/oversize_pdf_test_0.pdf to debug this. I'm still running tests, but patch 6 should have fixed this immediate issue. I managed to reproduce #23 and #25, but not #24. What URL are you using for #24 and are you scrolling at all?
> I managed to reproduce #23 and #25, but not #24. What URL are you using for #24 > and are you scrolling at all? #24 is easy to reproduce, but let me re-patch and see if that is still true. I'll take a screenshot if it is. (I don't remember right this second if scrolling was required) URLs are from my throttled local webserver. jbreiden3.mtv/100.pdf jbreiden3.mtv/101.pdf jbreiden3.mtv/102.pdf ....
Easy to reproduce. No scrolling required. Screenshot heading to your inbox.
On 2015/12/09 19:54:47, jab wrote: > Easy to reproduce. No scrolling required. Screenshot heading to your inbox. I think it's your web server. It doesn't seem to handle two request (from the same IP?) at the same time. Since the range requests are now much longer, it take longer before the new request chrome sends for the second PDF gets answered. I can reproduce this with wget on two terminals: term1$ wget http://jbreiden3.mtv.corp.google.com/101.pdf term2$ wget http://jbreiden3.mtv.corp.google.com/100.pdf The second request blocks until the first has finished.
Yes, probably a side effect from asked the local webserver to throttle itself. Okay, then maybe not something to worry about, and beyond the scope of this change. It's a little weird that Chrome does not break off the existing request when I change the URL and hit enter. I'm willing to ignore this.
On 2015/12/09 21:22:17, jab wrote: > Yes, probably a side effect from asked the local webserver to throttle itself. > > Okay, then maybe not something to worry about, and beyond the scope of this > change. It's a little weird that Chrome does not break off the existing request > when I change the URL and hit enter. I'm willing to ignore this. Can't find anything else wrong with this. Let me know if you still want to test it some more or if thestig@ wants to have another look. Otherwise I'll go ahead and submit to CQ.
The CQ bit was checked by spelchat@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from thestig@chromium.org, breidenbach@gmail.com Link to the patchset: https://codereview.chromium.org/1506023002/#ps100001 (title: "Fix issue with chunk_stream")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1506023002/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1506023002/100001
Message was sent while issue was closed.
Description was changed from ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201,78264 ========== to ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201,78264 ==========
Message was sent while issue was closed.
Committed patchset #6 (id:100001)
Message was sent while issue was closed.
Description was changed from ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201,78264 ========== to ========== Minimize the number of range requests made by PDFium Previously, Chrome would make a series of range requests for linearized PDFs, starting at 32 KB and slowly increasing. On high latency connections, this is considerably slower than using a single request. Now Chrome will make range requests as large as possible and cancel them if the renderer needs some data in a different place in the document. This significantly reduces the number of range requests performed by Chrome. BUG=460201,78264 Committed: https://crrev.com/3ba2a28104c4d6feef3efd71d9be73f085886f53 Cr-Commit-Position: refs/heads/master@{#364235} ==========
Message was sent while issue was closed.
Patchset 6 (id:??) landed as https://crrev.com/3ba2a28104c4d6feef3efd71d9be73f085886f53 Cr-Commit-Position: refs/heads/master@{#364235} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
