|
|
Created:
4 years, 3 months ago by Bryan McQuade Modified:
4 years, 3 months ago CC:
asvitkine+watch_chromium.org, chromium-reviews, csharrison+watch_chromium.org, loading-reviews+metrics_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd histograms for script execution time for doc write observer
There has been a desire to better understand the breakdown of performance
impact as a result of the doc.write intervention. We currently track
script load time duration for the intervention. This change allows us
to also track script execution time duration for the intervention.
BUG=640260
Committed: https://crrev.com/612a4b664c1589e12d2d12e35044c68224c63851
Cr-Commit-Position: refs/heads/master@{#419557}
Patch Set 1 #
Total comments: 7
Patch Set 2 : address histograms.xml comments #Patch Set 3 : move affected histograms to clients block #
Messages
Total messages: 31 (19 generated)
The CQ bit was checked by bmcquade@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...
bmcquade@chromium.org changed reviewers: + csharrison@chromium.org
Description was changed from ========== Add histograms for script execution time for doc write observer BUG=640260 ========== to ========== Add histograms for script execution time for doc write observer There has been a desire to better understand the breakdown of performance impact as a result of the doc.write intervention. We currently track script load time duration for the intervention. This change allows us to also track script execution time duration for the intervention. BUG=640260 ==========
bmcquade@chromium.org changed reviewers: + rkaplow@chromium.org
csharrison, PTAL for metrics change rkaplow, PTAL for histograms.xml
rkaplow@chromium.org changed reviewers: + gayane@chromium.org
lgtm https://codereview.chromium.org/2346813002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2346813002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:130: "ParseBlockedOnScriptExecutionFromDocumentWrite"; Have we dug ourselves into a hole of long histogram names with no way out? https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:103256: - ; Should histograms.xml presubmit be robust to this?
Thanks! https://codereview.chromium.org/2346813002/diff/1/chrome/browser/page_load_me... File chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc (right): https://codereview.chromium.org/2346813002/diff/1/chrome/browser/page_load_me... chrome/browser/page_load_metrics/observers/document_write_page_load_metrics_observer.cc:130: "ParseBlockedOnScriptExecutionFromDocumentWrite"; On 2016/09/15 at 15:28:59, Charlie Harrison wrote: > Have we dug ourselves into a hole of long histogram names with no way out? Yeah, I think these histograms are sufficiently specific that they require long names. It's not clear to me how to make these much shorter without losing important information in the histogram name. That said, it's only these 'leaf' histograms, that track specific information for a subset of page loads, that have to be this long. The core histograms are shorter, as they should be.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:103253: name="PageLoad.Clients.DocWrite.Block.Timing2.ParseDuration"/> Can the suffix name="Background" on line 104030 go to the top before all the affected histograms are defined ? https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:103404: + name="PageLoad.ParseTiming.ParseBlockedOnScriptExecutionFromDocumentWrite"/> should these be PageLoad.Clients.DocWrite.Evaluator.ParseTiming.* instead of PageLoad.ParseTiming.*
The CQ bit was checked by bmcquade@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...
The CQ bit was checked by bmcquade@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 for the review! PTAL. https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:103253: name="PageLoad.Clients.DocWrite.Block.Timing2.ParseDuration"/> On 2016/09/16 at 16:59:43, gayane wrote: > Can the suffix name="Background" on line 104030 go to the top before all the affected histograms are defined ? ah, yes, not sure how things got re-ordered like this. moved it, thanks! https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2346813002/diff/1/tools/metrics/histograms/hi... tools/metrics/histograms/histograms.xml:103404: + name="PageLoad.ParseTiming.ParseBlockedOnScriptExecutionFromDocumentWrite"/> On 2016/09/16 at 16:59:43, gayane wrote: > should these be PageLoad.Clients.DocWrite.Evaluator.ParseTiming.* instead of PageLoad.ParseTiming.* ah, my mistake, i put the affected-histograms in the wrong block, thanks! i've moved this to the 'clients' block. i also re-ordered this block to put the <suffix> entries at the top.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm
The CQ bit was checked by bmcquade@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from csharrison@chromium.org Link to the patchset: https://codereview.chromium.org/2346813002/#ps40001 (title: "move affected histograms to clients block")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: chromium_presubmit on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/chromium_presub...)
The CQ bit was checked by gayane@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Description was changed from ========== Add histograms for script execution time for doc write observer There has been a desire to better understand the breakdown of performance impact as a result of the doc.write intervention. We currently track script load time duration for the intervention. This change allows us to also track script execution time duration for the intervention. BUG=640260 ========== to ========== Add histograms for script execution time for doc write observer There has been a desire to better understand the breakdown of performance impact as a result of the doc.write intervention. We currently track script load time duration for the intervention. This change allows us to also track script execution time duration for the intervention. BUG=640260 Committed: https://crrev.com/612a4b664c1589e12d2d12e35044c68224c63851 Cr-Commit-Position: refs/heads/master@{#419557} ==========
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/612a4b664c1589e12d2d12e35044c68224c63851 Cr-Commit-Position: refs/heads/master@{#419557} |