Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2078113002:
Don't start a second pre-render if one is in progress. (Closed)
|
Created:
4 years, 6 months ago by Pete Williamson Modified:
4 years, 6 months ago CC:
chromium-reviews, romax+watch_chromium.org, fgorski+watch_chromium.org, dewittj+watch_chromium.org, petewil+watch_chromium.org, chili+watch_chromium.org, dimich+watch_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionDon't start a second pre-render if one is in progress.
The prerenderer can only support one prerender attempt at a time.
If we are asked to start another, return false instead of starting one.
In normal operation, we will take a request off the queue when
prerendering finishes anyway.
BUG=610521
Committed: https://crrev.com/0a866ec6eee17369a8fe3f565c55937fd3bf985b
Cr-Commit-Position: refs/heads/master@{#401461}
Patch Set 1 #
Total comments: 16
Patch Set 2 : CR fixes per DougArnett, Chili, and FGorski #
Total comments: 2
Patch Set 3 : fix comment #
Total comments: 2
Patch Set 4 : Redo unit test to use StartProcessing #
Messages
Total messages: 26 (8 generated)
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078113002/1
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
petewil@chromium.org changed reviewers: + dougarnett@chromium.org, fgorski@chromium.org
Couple ideas to avoid more friend dependencies on the unittests class. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:97: return coordinator_->request_in_progress_; Might consider public method on Coordinator instead (IsBusy()) vs. adding another friend dependency. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:192: SendRequestToOffliner(request); Why not call StartProcessing (with some PumpLoops and a request on the queue) to make it busy instead? You could then also have another variant where there is no request to be picked that would not make it busy.
https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:99: bool request_in_progress_; though probably annoying to change - can we rename to is_request_in_progress_ to more easily tell this is a boolean? 'request_in_progress' to me implies that this is the actual request that is processing
mostly nits on my end. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:89: if (request_in_progress_) { nit: {} are not necessary here. (not required by the style guide). As a team we agreed in the past to not surround one liners in C++ https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:124: DCHECK(request_in_progress_ == false); nit: DCHECK(!request_in_progress_); https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:140: request_in_progress_ = false; do you think it makes sense to DCHECK(request_in_progress_) here?
https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:89: if (request_in_progress_) { On 2016/06/20 20:24:07, fgorski wrote: > nit: {} are not necessary here. (not required by the style guide). As a team we > agreed in the past to not surround one liners in C++ Done. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:124: DCHECK(request_in_progress_ == false); On 2016/06/20 20:24:07, fgorski wrote: > nit: DCHECK(!request_in_progress_); Done. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:140: request_in_progress_ = false; On 2016/06/20 20:24:07, fgorski wrote: > do you think it makes sense to DCHECK(request_in_progress_) here? Done. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.h:99: bool request_in_progress_; On 2016/06/20 18:38:34, chili wrote: > though probably annoying to change - can we rename to is_request_in_progress_ to > more easily tell this is a boolean? 'request_in_progress' to me implies that > this is the actual request that is processing renamed to is_busy_ https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:97: return coordinator_->request_in_progress_; On 2016/06/20 18:30:08, dougarnett wrote: > Might consider public method on Coordinator instead (IsBusy()) vs. adding > another friend dependency. New public method is_busy() added. (Following accessor naming conventions, I named it after the variable name), which I changed to be inline with your and chili's comments. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:192: SendRequestToOffliner(request); On 2016/06/20 18:30:08, dougarnett wrote: > Why not call StartProcessing (with some PumpLoops and a request on the queue) to > make it busy instead? > > You could then also have another variant where there is no request to be picked > that would not make it busy. I tried that, but the process goes all the way through the fake offliner, which calls back to end the request being in-progress before I start the second request. If I remove the behavior of calling the callback, other tests fail. I could just make the test stub for the offliner more complicated so that I can tell it to not call back in this case, but we don't have a pointer to the offliner that the test can access (only the factory). I could modify things further so we can capture a pointer to the stub offliner that the test can use. It seems overall simpler to me to run only the code being tested here instead of the whole system by leaving the code as is (calling SendRequestToOffliner to set busy, and calling StartProcessing to check the is_busy flag.) Note that we already test the path for no request already in progress in the test above (no changes were required to test it). So, what do you think about leaving this part as is?
https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator.cc:140: request_in_progress_ = false; On 2016/06/21 17:55:25, Pete Williamson wrote: > On 2016/06/20 20:24:07, fgorski wrote: > > do you think it makes sense to DCHECK(request_in_progress_) here? > > Done. Oops, I did it and then undid it. I undid it since the DCHECK was firing during tests. I think the other DCHECK provides sufficient protection.
lgtm mod comment. https://codereview.chromium.org/2078113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2078113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:103: bool is_busy_; please add documentation here. What does is_busy means? I think is_request_in_progress_ had better chances of being self documenting. With is_busy you need to put in more work.
https://codereview.chromium.org/2078113002/diff/20001/components/offline_page... File components/offline_pages/background/request_coordinator.h (right): https://codereview.chromium.org/2078113002/diff/20001/components/offline_page... components/offline_pages/background/request_coordinator.h:103: bool is_busy_; On 2016/06/21 18:11:18, fgorski wrote: > please add documentation here. What does is_busy means? > > I think is_request_in_progress_ had better chances of being self documenting. > With is_busy you need to put in more work. Done.
I tried to explain my test improvement idea better. https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:192: SendRequestToOffliner(request); On 2016/06/21 17:55:25, Pete Williamson wrote: > On 2016/06/20 18:30:08, dougarnett wrote: > > Why not call StartProcessing (with some PumpLoops and a request on the queue) > to > > make it busy instead? > > > > You could then also have another variant where there is no request to be > picked > > that would not make it busy. > > I tried that, but the process goes all the way through the fake offliner, which > calls back to end the request being in-progress before I start the second > request. If I remove the behavior of calling the callback, other tests fail. I > could just make the test stub for the offliner more complicated so that I can > tell it to not call back in this case, but we don't have a pointer to the > offliner that the test can access (only the factory). I could modify things > further so we can capture a pointer to the stub offliner that the test can use. > > It seems overall simpler to me to run only the code being tested here instead of > the whole system by leaving the code as is (calling SendRequestToOffliner to set > busy, and calling StartProcessing to check the is_busy flag.) > > Note that we already test the path for no request already in progress in the > test above (no changes were required to test it). > > So, what do you think about leaving this part as is? Ah, so that is actually a good case that I wasn't actually commenting about here - that a processing request gets cleared and then StartProcessing accepts again. My idea here did not involve doing anything further with Offliner. Instead it is just about testing via the StartProcessing interface (and let's ignore my other variant suggestion for now): StartProcessingWithRequestInProgress 1. SavePageLater - to enqueue request to memory queue 2. StartProcessing returns true (since have queued request) 3. StartProcessing returns false (have queued request but is_busy()) Then your idea could be added later: 4. mock processing done callback 5. StartProcessing returns true The 1,2,3 approach seems preferable to me in order to both reason about testing the behavior from the public api; and exercises more internal code paths. Unless I am missing something, this is just at the cost/complexity of calling another public api (SavePageLater) and using memory request queue. https://codereview.chromium.org/2078113002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:193: EXPECT_TRUE(request_in_progress()); suggest using the is_busy() naming for the test class as well - to be consistent
https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:192: SendRequestToOffliner(request); On 2016/06/22 17:25:47, dougarnett wrote: > On 2016/06/21 17:55:25, Pete Williamson wrote: > > On 2016/06/20 18:30:08, dougarnett wrote: > > > Why not call StartProcessing (with some PumpLoops and a request on the > queue) > > to > > > make it busy instead? > > > > > > You could then also have another variant where there is no request to be > > picked > > > that would not make it busy. > > > > I tried that, but the process goes all the way through the fake offliner, > which > > calls back to end the request being in-progress before I start the second > > request. If I remove the behavior of calling the callback, other tests fail. > I > > could just make the test stub for the offliner more complicated so that I can > > tell it to not call back in this case, but we don't have a pointer to the > > offliner that the test can access (only the factory). I could modify things > > further so we can capture a pointer to the stub offliner that the test can > use. > > > > It seems overall simpler to me to run only the code being tested here instead > of > > the whole system by leaving the code as is (calling SendRequestToOffliner to > set > > busy, and calling StartProcessing to check the is_busy flag.) > > > > Note that we already test the path for no request already in progress in the > > test above (no changes were required to test it). > > > > So, what do you think about leaving this part as is? > > Ah, so that is actually a good case that I wasn't actually commenting about here > - that a processing request gets cleared and then StartProcessing accepts again. > > My idea here did not involve doing anything further with Offliner. Instead it is > just about testing via the StartProcessing interface (and let's ignore my other > variant suggestion for now): > > StartProcessingWithRequestInProgress > 1. SavePageLater - to enqueue request to memory queue > 2. StartProcessing returns true (since have queued request) > 3. StartProcessing returns false (have queued request but is_busy()) > > Then your idea could be added later: > 4. mock processing done callback > 5. StartProcessing returns true > > The 1,2,3 approach seems preferable to me in order to both reason about testing > the behavior from the public api; and exercises more internal code paths. Unless > I am missing something, this is just at the cost/complexity of calling another > public api (SavePageLater) and using memory request queue. If I understand your suggestion correctly, then between step 2 and 3, the is_busy_ variable will not get set because the callbacks need to run before it gets set by SendReqeustToOffliner(). However, if the callbacks run, then the stub offliner will actually report that it is done, clearing the is_busy_ variable again, so the call in step 3 will see nothing as in progress, and the test won't work. So, I just modified the existing StubOffliner object to have a way to skip the final "offliner done" callback so that the is_busy_ variable never gets cleared, which is what I said I would need to do previously. https://codereview.chromium.org/2078113002/diff/40001/components/offline_page... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/40001/components/offline_page... components/offline_pages/background/request_coordinator_unittest.cc:193: EXPECT_TRUE(request_in_progress()); On 2016/06/22 17:25:47, dougarnett wrote: > suggest using the is_busy() naming for the test class as well - to be consistent Done.
The CQ bit was checked by petewil@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078113002/60001
lgtm https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... File components/offline_pages/background/request_coordinator_unittest.cc (right): https://codereview.chromium.org/2078113002/diff/1/components/offline_pages/ba... components/offline_pages/background/request_coordinator_unittest.cc:192: SendRequestToOffliner(request); On 2016/06/22 22:32:26, Pete Williamson wrote: > On 2016/06/22 17:25:47, dougarnett wrote: > > On 2016/06/21 17:55:25, Pete Williamson wrote: > > > On 2016/06/20 18:30:08, dougarnett wrote: > > > > Why not call StartProcessing (with some PumpLoops and a request on the > > queue) > > > to > > > > make it busy instead? > > > > > > > > You could then also have another variant where there is no request to be > > > picked > > > > that would not make it busy. > > > > > > I tried that, but the process goes all the way through the fake offliner, > > which > > > calls back to end the request being in-progress before I start the second > > > request. If I remove the behavior of calling the callback, other tests > fail. > > I > > > could just make the test stub for the offliner more complicated so that I > can > > > tell it to not call back in this case, but we don't have a pointer to the > > > offliner that the test can access (only the factory). I could modify things > > > further so we can capture a pointer to the stub offliner that the test can > > use. > > > > > > It seems overall simpler to me to run only the code being tested here > instead > > of > > > the whole system by leaving the code as is (calling SendRequestToOffliner to > > set > > > busy, and calling StartProcessing to check the is_busy flag.) > > > > > > Note that we already test the path for no request already in progress in the > > > test above (no changes were required to test it). > > > > > > So, what do you think about leaving this part as is? > > > > Ah, so that is actually a good case that I wasn't actually commenting about > here > > - that a processing request gets cleared and then StartProcessing accepts > again. > > > > My idea here did not involve doing anything further with Offliner. Instead it > is > > just about testing via the StartProcessing interface (and let's ignore my > other > > variant suggestion for now): > > > > StartProcessingWithRequestInProgress > > 1. SavePageLater - to enqueue request to memory queue > > 2. StartProcessing returns true (since have queued request) > > 3. StartProcessing returns false (have queued request but is_busy()) > > > > Then your idea could be added later: > > 4. mock processing done callback > > 5. StartProcessing returns true > > > > The 1,2,3 approach seems preferable to me in order to both reason about > testing > > the behavior from the public api; and exercises more internal code paths. > Unless > > I am missing something, this is just at the cost/complexity of calling another > > public api (SavePageLater) and using memory request queue. > > If I understand your suggestion correctly, then between step 2 and 3, the > is_busy_ variable will not get set because the callbacks need to run before it > gets set by SendReqeustToOffliner(). However, if the callbacks run, then the > stub offliner will actually report that it is done, clearing the is_busy_ > variable again, so the call in step 3 will see nothing as in progress, and the > test won't work. > > So, I just modified the existing StubOffliner object to have a way to skip the > final "offliner done" callback so that the is_busy_ variable never gets cleared, > which is what I said I would need to do previously. I see your point with OfflinerStub now - I missed the automatic callback.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
The CQ bit was checked by petewil@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from fgorski@chromium.org Link to the patchset: https://codereview.chromium.org/2078113002/#ps60001 (title: "Redo unit test to use StartProcessing")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/2078113002/60001
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== Don't start a second pre-render if one is in progress. The prerenderer can only support one prerender attempt at a time. If we are asked to start another, return false instead of starting one. In normal operation, we will take a request off the queue when prerendering finishes anyway. BUG=610521 ========== to ========== Don't start a second pre-render if one is in progress. The prerenderer can only support one prerender attempt at a time. If we are asked to start another, return false instead of starting one. In normal operation, we will take a request off the queue when prerendering finishes anyway. BUG=610521 Committed: https://crrev.com/0a866ec6eee17369a8fe3f565c55937fd3bf985b Cr-Commit-Position: refs/heads/master@{#401461} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/0a866ec6eee17369a8fe3f565c55937fd3bf985b Cr-Commit-Position: refs/heads/master@{#401461} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
