Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2581043003:
Add page-level aggregation of SubresourceFilter time metrics. (Closed)
|
Created:
4 years ago by pkalinnikov Modified:
3 years, 11 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, darin-cc_chromium.org, jam, mlamouri+watch-content_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd page-level aggregation of SubresourceFilter time metrics.
Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent
to a RenderFrameHost in the browser when a document load is finished, just
before the DidFinishLoad message, if performance measurements were enabled for
the load. Contains the total time spent on evaluating subresource loads in
DocumentSubresourceFilter::allowLoad() for a frame.
Measurements are aggregated in SubresourceFilterDriverFactory and recorded to
"SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when
DidFinishLoad is called for the main frame.
BUG=609747, 672519
Committed: https://crrev.com/22be4914542ba9072ee2e6c7c3480881ebcaa6fe
Cr-Commit-Position: refs/heads/master@{#440849}
Patch Set 1 #
Total comments: 23
Patch Set 2 : Address comments of engedy@ and add tests. #
Total comments: 18
Patch Set 3 : Address more comments from engedy@. #
Total comments: 19
Patch Set 4 : Address nits. #
Total comments: 2
Patch Set 5 : Rebase. Make tests work with and without PersistentHistograms. #Dependent Patchsets: Messages
Total messages: 38 (21 generated)
pkalinnikov@chromium.org changed reviewers: + engedy@chromium.org
Balazs, PTAL. I will add tests later. Please verify the approach, and maybe suggest where it is better to test. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:225: if (activation_state_ != ActivationState::DISABLED && measure_performance_) { Probably just need to check |measure_performance_|, but set it to true below only if |activation_state_| != DISABLED. WDYT?
Looking good so far. Regarding testing, we should have: -- some basic tests in subresource_filter_agent_unittest, -- a browsertest with base::HistogramTester in chrome/browser/subresource_filter to verify everything end-to-end. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:37: bool ShouldMeasurePerformanceForThisPage() { nit: s/ThisPage/PageLoad/ https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:41: return rate == 1 || (rate > 0 && base::RandDouble() < rate); Isn't this equivalent to just `base::RandDouble() < rate`? https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:225: if (activation_state_ != ActivationState::DISABLED && measure_performance_) { On 2016/12/16 16:54:23, pkalinnikov wrote: > Probably just need to check |measure_performance_|, but set it to true below > only if |activation_state_| != DISABLED. WDYT? Sounds good. Then you can add this as a DCHECK to document the assumption. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:145: // Total time spent in DocumentSubresourceFilter::allowLoad() calls while nit: .... calls, aggregated across all frames, evaluating ... https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:146: // evaluating subresource loads for the current document. nit: s/document/page load/ https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/common/subresource_filter_messages.h (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/common/subresource_filter_messages.h:43: // Sent to the browser the first time a subresource load is disallowed for the Should mention here that this is sent when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/common/subresource_filter_messages.h:51: IPC_MESSAGE_ROUTED2(SubresourceFilterHostMsg_DidAggregateEvaluationDuration, What do you think about: _DocumentLoadStatistics https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/renderer/subresource_filter_agent.cc (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/subresource_filter_agent.cc:117: render_frame()->Send( Note that the unit test for this class is quite tricky. There won't be a render frame in the unittest, see explanation in: https://cs.chromium.org/chromium/src/components/subresource_filter/content/re... This means that we will need to put this into a virtual method so that it can be mocked out. It probably makes sense to either add the reported durations as arguments so they can be verified during tests, or find another way to verify them. https://codereview.chromium.org/2581043003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2581043003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:64368: + DocumentSubresourceFilters on processing subresource requests in allowLoad. Phrasing nit (same below): ... spent on evaluating subresource requests, aggregated across all frames.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Addressed comments and added tests. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:37: bool ShouldMeasurePerformanceForThisPage() { On 2016/12/19 10:18:42, engedy wrote: > nit: s/ThisPage/PageLoad/ Done. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:40: static const double rate = GetPerformanceMeasurementRate(); static local turned out to be a bad idea, because in tests variation params are changed dynamically. I leave a TODO to address this in a follow-up CL. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:41: return rate == 1 || (rate > 0 && base::RandDouble() < rate); On 2016/12/19 10:18:42, engedy wrote: > Isn't this equivalent to just `base::RandDouble() < rate`? Right, but I did this in order to optimize for 0 and 1 cases (by not calling RandDouble), because those are the most anticipated values of |rate|. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:225: if (activation_state_ != ActivationState::DISABLED && measure_performance_) { On 2016/12/19 10:18:42, engedy wrote: > On 2016/12/16 16:54:23, pkalinnikov wrote: > > Probably just need to check |measure_performance_|, but set it to true below > > only if |activation_state_| != DISABLED. WDYT? > > Sounds good. Then you can add this as a DCHECK to document the assumption. Done. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:145: // Total time spent in DocumentSubresourceFilter::allowLoad() calls while On 2016/12/19 10:18:42, engedy wrote: > nit: .... calls, aggregated across all frames, evaluating ... Done. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.h:146: // evaluating subresource loads for the current document. On 2016/12/19 10:18:42, engedy wrote: > nit: s/document/page load/ Done. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/common/subresource_filter_messages.h (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/common/subresource_filter_messages.h:43: // Sent to the browser the first time a subresource load is disallowed for the On 2016/12/19 10:18:42, engedy wrote: > Should mention here that this is sent when a document load is finished, just > before the DidFinishLoad message, if performance measurements were enabled for > the load. Done. I assume, that you meant not here but below. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/common/subresource_filter_messages.h:51: IPC_MESSAGE_ROUTED2(SubresourceFilterHostMsg_DidAggregateEvaluationDuration, On 2016/12/19 10:18:42, engedy wrote: > What do you think about: _DocumentLoadStatistics Done. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/renderer/subresource_filter_agent.cc (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/renderer/subresource_filter_agent.cc:117: render_frame()->Send( On 2016/12/19 10:18:42, engedy wrote: > Note that the unit test for this class is quite tricky. There won't be a render > frame in the unittest, see explanation in: > > https://cs.chromium.org/chromium/src/components/subresource_filter/content/re... > > This means that we will need to put this into a virtual method so that it can be > mocked out. It probably makes sense to either add the reported durations as > arguments so they can be verified during tests, or find another way to verify > them. Done. https://codereview.chromium.org/2581043003/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2581043003/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:64368: + DocumentSubresourceFilters on processing subresource requests in allowLoad. On 2016/12/19 10:18:42, engedy wrote: > Phrasing nit (same below): > > ... spent on evaluating subresource requests, aggregated across all frames. Done.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thank you, looks great, LGTM! It's awesome to finally have some end-to-end test for histograms. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:40: static const double rate = GetPerformanceMeasurementRate(); On 2016/12/21 15:28:46, pkalinnikov wrote: > static local turned out to be a bad idea, because in tests variation params are > changed dynamically. I leave a TODO to address this in a follow-up CL. Acknowledged. Recommended sounds good too. https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:41: return rate == 1 || (rate > 0 && base::RandDouble() < rate); On 2016/12/21 15:28:46, pkalinnikov wrote: > On 2016/12/19 10:18:42, engedy wrote: > > Isn't this equivalent to just `base::RandDouble() < rate`? > > Right, but I did this in order to optimize for 0 and 1 cases (by not calling > RandDouble), because those are the most anticipated values of |rate|. Ack, could you please add a one-liner comment to point this out? https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... File components/subresource_filter/content/common/subresource_filter_messages.h (right): https://codereview.chromium.org/2581043003/diff/1/components/subresource_filt... components/subresource_filter/content/common/subresource_filter_messages.h:43: // Sent to the browser the first time a subresource load is disallowed for the On 2016/12/21 15:28:46, pkalinnikov wrote: > On 2016/12/19 10:18:42, engedy wrote: > > Should mention here that this is sent when a document load is finished, just > > before the DidFinishLoad message, if performance measurements were enabled for > > the load. > > Done. I assume, that you meant not here but below. Acknowledged. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:276: constexpr const char kPathToFrameSet[] = "subresource_filter/frame_set.html"; nit: This is used further up in the file in multiple places, let's reuse this global constant then. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:279: constexpr base::Histogram::Count number_of_pages = 1; nit: kNumberOfPages nit: Is there any real simple way we could encapsulate these constants somehow better with the frame set path? https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:306: ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix("example")); nit: Please use `suffix-that-does-not-match-anything`, otherwise it may be unclear to the reader what magic role the world `example` plays. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:312: // The following histograms are generated on the browser side. Could you please also add checks for: SubresourceFilter.DocumentLoad.ActivationState (may overcount, if it does, use the current value as expectation and leave a TODO for me). SubresourceFilter.DocumentLoad.NumSubresourceLoads.* https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:247: OnEvaluationDurationAggregated) nit: Update method name. https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/common/subresource_filter_messages.h (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/common/subresource_filter_messages.h:50: // were enabled for the load. Contains total time spent on evaluating nit: the total https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/subresource_filter_agent.h (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/renderer/subresource_filter_agent.h:59: // Informs the browser about the time spend by DocumentSubresourceFilter in nit: spent https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:446: TEST_F(SubresourceFilterAgentTest, SendDocumentLoadStatistics) { Could you please add a comment that describes what this test verifies -- in addition compared to the test above? https://codereview.chromium.org/2581043003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2581043003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64378: + records the total real time spent on evaluating subresource requests nit: Comma at end of line.
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Balazs, please take another look. Pay attention to the questions in the browsertest. I will investigate these more next week. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:276: constexpr const char kPathToFrameSet[] = "subresource_filter/frame_set.html"; On 2016/12/21 19:10:28, engedy wrote: > nit: This is used further up in the file in multiple places, let's reuse this > global constant then. Done. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:279: constexpr base::Histogram::Count number_of_pages = 1; On 2016/12/21 19:10:28, engedy wrote: > nit: kNumberOfPages > nit: Is there any real simple way we could encapsulate these constants somehow > better with the frame set path? Done. The encapsulation nit is probably not relevant any more. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:306: ASSERT_NO_FATAL_FAILURE(SetRulesetToDisallowURLsWithPathSuffix("example")); On 2016/12/21 19:10:28, engedy wrote: > nit: Please use `suffix-that-does-not-match-anything`, otherwise it may be > unclear to the reader what magic role the world `example` plays. Done. https://codereview.chromium.org/2581043003/diff/20001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:312: // The following histograms are generated on the browser side. On 2016/12/21 19:10:28, engedy wrote: > Could you please also add checks for: > > SubresourceFilter.DocumentLoad.ActivationState (may overcount, if it does, use > the current value as expectation and leave a TODO for me). > > SubresourceFilter.DocumentLoad.NumSubresourceLoads.* Done. https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/browser/content_subresource_filter_driver_factory.cc:247: OnEvaluationDurationAggregated) On 2016/12/21 19:10:28, engedy wrote: > nit: Update method name. Done. https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/common/subresource_filter_messages.h (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/common/subresource_filter_messages.h:50: // were enabled for the load. Contains total time spent on evaluating On 2016/12/21 19:10:28, engedy wrote: > nit: the total Done. https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/subresource_filter_agent.h (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/renderer/subresource_filter_agent.h:59: // Informs the browser about the time spend by DocumentSubresourceFilter in On 2016/12/21 19:10:28, engedy wrote: > nit: spent Done. https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... File components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc (right): https://codereview.chromium.org/2581043003/diff/20001/components/subresource_... components/subresource_filter/content/renderer/subresource_filter_agent_unittest.cc:446: TEST_F(SubresourceFilterAgentTest, SendDocumentLoadStatistics) { On 2016/12/21 19:10:28, engedy wrote: > Could you please add a comment that describes what this test verifies -- in > addition compared to the test above? What was different: 1. This test was distilled to check only the LoadStatistics call. 2. The order of requests was slightly changed, so that the first request wasn't producing the first-disallowed event. As discussed, there is no much sense in separating these. Let me just delete the test and also change the order of requests in the one above. https://codereview.chromium.org/2581043003/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2581043003/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:64378: + records the total real time spent on evaluating subresource requests On 2016/12/21 19:10:28, engedy wrote: > nit: Comma at end of line. Done. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:331: time_recorded ? 6 : 0); Not sure why the count is only 6 here and below for Activation*Duration histograms. Shouldn't it be 7 together with the main frame? https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:335: tester.ExpectTotalCount(kEvaluationWallDuration, time_recorded ? 7 : 0); These two are overcounted because of double allowLoad calls: in blink's preload() and fetch(). https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:349: ::testing::ElementsAre(base::Bucket(0, 3), base::Bucket(2, 3))); These two also have weird numbers. Bucket(2, 3) means that the number of subresources has doubled. Not clear why there are 3 zeroes, I would expect only one, coming from "frame_with_no_subresources.html". https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:356: ::testing::ElementsAre(base::Bucket(0, 4), base::Bucket(2, 2))); These are also not clear.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Still LGTM % nits, see answers below. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:251: const char* kSubframeNames[] = {"one", "three"}; nit: Instead of the special casing below, what do you think about: kSubframeNames = {"one", "two", "three"} kExpectScriptElementToLoad = {false, true, false} + a single loop https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:331: time_recorded ? 6 : 0); On 2016/12/22 16:53:04, pkalinnikov wrote: > Not sure why the count is only 6 here and below for Activation*Duration > histograms. Shouldn't it be 7 together with the main frame? Note that the dynamic frame is not created unless InsertDynamicFrameWithScript() is called -- so there should be the mainframe and the 5 subframes only, so this looks correct, unless I am missing something. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:335: tester.ExpectTotalCount(kEvaluationWallDuration, time_recorded ? 7 : 0); On 2016/12/22 16:53:04, pkalinnikov wrote: > These two are overcounted because of double allowLoad calls: in blink's > preload() and fetch(). Then the +1 comes from the load to /favicon.ico. Because this comes after the load event in the main frame, it's only counted towards this histogram, and not the DocumentLoad* or PageLoad* aggregates. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:342: // TODO(engedy): Figure out why one DISABLED activation state (for an empty So that looks like the initial main-frame navigation to about:blank. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:349: ::testing::ElementsAre(base::Bucket(0, 3), base::Bucket(2, 3))); On 2016/12/22 16:53:04, pkalinnikov wrote: > These two also have weird numbers. Bucket(2, 3) means that the number of > subresources has doubled. Not clear why there are 3 zeroes, I would expect only > one, coming from "frame_with_no_subresources.html". Hmm, this sounds plausible too: the "frame_with_delayed_script.html" will not inject a dynamic script tag unless IsDynamicScriptElementLoaded() is called. Then there is main frame that has no non-frame subresources. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:356: ::testing::ElementsAre(base::Bucket(0, 4), base::Bucket(2, 2))); On 2016/12/22 16:53:04, pkalinnikov wrote: > These are also not clear. We have "frame_with_delayed_script.html" twice with the script disallowed, one being the preload, the other the actual load. So this is once again plausible. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:382: ExpectHistogramsAreRecordedForTestFrameSet(tester, true); nit: Let's name the argument |expect_performance_measurements| to indicate that it's not changing behavior, only setting expectations. You could also indicate the meaning of `true` at the call site: (tester, true /* expect_performance_measurements */) Otherwise, nice solution to avoid duplication!
Description was changed from ========== Add page-level aggregation of SubresourceFilter time metrics. BUG=609747 ========== to ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747 ==========
https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:251: const char* kSubframeNames[] = {"one", "three"}; On 2016/12/23 10:34:26, engedy wrote: > nit: Instead of the special casing below, what do you think about: > > kSubframeNames = {"one", "two", "three"} > kExpectScriptElementToLoad = {false, true, false} > > + a single loop Done. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:331: time_recorded ? 6 : 0); On 2016/12/23 10:34:26, engedy wrote: > On 2016/12/22 16:53:04, pkalinnikov wrote: > > Not sure why the count is only 6 here and below for Activation*Duration > > histograms. Shouldn't it be 7 together with the main frame? > > Note that the dynamic frame is not created unless InsertDynamicFrameWithScript() > is called -- so there should be the mainframe and the 5 subframes only, so this > looks correct, unless I am missing something. Acknowledged. Do you think it makes sense to add InsertDynamicFrameWithScript to this test? https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:335: tester.ExpectTotalCount(kEvaluationWallDuration, time_recorded ? 7 : 0); On 2016/12/23 10:34:26, engedy wrote: > On 2016/12/22 16:53:04, pkalinnikov wrote: > > These two are overcounted because of double allowLoad calls: in blink's > > preload() and fetch(). > > Then the +1 comes from the load to /favicon.ico. Because this comes after the > load event in the main frame, it's only counted towards this histogram, and not > the DocumentLoad* or PageLoad* aggregates. Acknowledged. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:342: // TODO(engedy): Figure out why one DISABLED activation state (for an empty On 2016/12/23 10:34:26, engedy wrote: > So that looks like the initial main-frame navigation to about:blank. Added expectation and comment. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:349: ::testing::ElementsAre(base::Bucket(0, 3), base::Bucket(2, 3))); On 2016/12/23 10:34:26, engedy wrote: > On 2016/12/22 16:53:04, pkalinnikov wrote: > > These two also have weird numbers. Bucket(2, 3) means that the number of > > subresources has doubled. Not clear why there are 3 zeroes, I would expect > only > > one, coming from "frame_with_no_subresources.html". > > Hmm, this sounds plausible too: the "frame_with_delayed_script.html" will not > inject a dynamic script tag unless IsDynamicScriptElementLoaded() is called. > Then there is main frame that has no non-frame subresources. Acknowledged. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:356: ::testing::ElementsAre(base::Bucket(0, 4), base::Bucket(2, 2))); On 2016/12/23 10:34:26, engedy wrote: > On 2016/12/22 16:53:04, pkalinnikov wrote: > > These are also not clear. > > We have "frame_with_delayed_script.html" twice with the script disallowed, one > being the preload, the other the actual load. So this is once again plausible. Acknowledged. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:382: ExpectHistogramsAreRecordedForTestFrameSet(tester, true); On 2016/12/23 10:34:26, engedy wrote: > nit: Let's name the argument |expect_performance_measurements| to indicate that > it's not changing behavior, only setting expectations. > > You could also indicate the meaning of `true` at the call site: > > (tester, true /* expect_performance_measurements */) > > Otherwise, nice solution to avoid duplication! Done.
LGTM. Let's find some OWNERS. https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/40001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:331: time_recorded ? 6 : 0); On 2016/12/27 11:57:52, pkalinnikov wrote: > On 2016/12/23 10:34:26, engedy wrote: > > On 2016/12/22 16:53:04, pkalinnikov wrote: > > > Not sure why the count is only 6 here and below for Activation*Duration > > > histograms. Shouldn't it be 7 together with the main frame? > > > > Note that the dynamic frame is not created unless > InsertDynamicFrameWithScript() > > is called -- so there should be the mainframe and the 5 subframes only, so > this > > looks correct, unless I am missing something. > > Acknowledged. Do you think it makes sense to add InsertDynamicFrameWithScript to > this test? It would not change the metrics as it happens after the load event, so let's just keep it as it is.
pkalinnikov@chromium.org changed reviewers: + isherman@chromium.org, palmer@chromium.org
Hi Chris and Ilya, palmer@: Please review the new IPC message in "subresource_filter_messages.h". isherman@: Please review "histograms.xml".
https://codereview.chromium.org/2581043003/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:328: SubprocessMetricsProvider::MergeHistogramDeltasForTesting(); TIL that you need to call content::FetchHistogramsFromChildProcesses() here as well, see https://codereview.chromium.org/2603773002/.
lgtm
Description was changed from ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747 ========== to ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747,672519 ==========
metrics lgtm, thanks
The CQ bit was checked by pkalinnikov@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Thanks! https://codereview.chromium.org/2581043003/diff/60001/chrome/browser/subresou... File chrome/browser/subresource_filter/subresource_filter_browsertest.cc (right): https://codereview.chromium.org/2581043003/diff/60001/chrome/browser/subresou... chrome/browser/subresource_filter/subresource_filter_browsertest.cc:328: SubprocessMetricsProvider::MergeHistogramDeltasForTesting(); On 2016/12/27 17:20:11, engedy wrote: > TIL that you need to call content::FetchHistogramsFromChildProcesses() here as > well, see https://codereview.chromium.org/2603773002/. Done.
LGTM.
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 pkalinnikov@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from palmer@chromium.org, isherman@chromium.org Link to the patchset: https://codereview.chromium.org/2581043003/#ps80001 (title: "Rebase. Make tests work with and without PersistentHistograms.")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 80001, "attempt_start_ts": 1482934775579830,
"parent_rev": "0c59934049320bc580631c2be9594a0cc7569b4d", "commit_rev":
"591a7cc368a35579f1e8ea53c88ca85ad8e18c00"}
Message was sent while issue was closed.
Description was changed from ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747,672519 ========== to ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747,672519 Review-Url: https://codereview.chromium.org/2581043003 ==========
Message was sent while issue was closed.
Committed patchset #5 (id:80001)
Message was sent while issue was closed.
Description was changed from ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747,672519 Review-Url: https://codereview.chromium.org/2581043003 ========== to ========== Add page-level aggregation of SubresourceFilter time metrics. Add a new message (SubresourceFilterHostMsg_DocumentLoadStatistics) that is sent to a RenderFrameHost in the browser when a document load is finished, just before the DidFinishLoad message, if performance measurements were enabled for the load. Contains the total time spent on evaluating subresource loads in DocumentSubresourceFilter::allowLoad() for a frame. Measurements are aggregated in SubresourceFilterDriverFactory and recorded to "SubresourceFilter.PageLoad.SubresourceEvaluation.*Duration" histograms when DidFinishLoad is called for the main frame. BUG=609747,672519 Committed: https://crrev.com/22be4914542ba9072ee2e6c7c3480881ebcaa6fe Cr-Commit-Position: refs/heads/master@{#440849} ==========
Message was sent while issue was closed.
Patchset 5 (id:??) landed as https://crrev.com/22be4914542ba9072ee2e6c7c3480881ebcaa6fe Cr-Commit-Position: refs/heads/master@{#440849} |
