Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2364923004:
Add UMA histograms to MHTML save operations. (Closed)
|
Created:
4 years, 3 months ago by carlosk Modified:
4 years, 2 months ago CC:
asanka, asvitkine+watch_chromium.org, blink-reviews, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, dcheng, jam, kinuko+watch, Łukasz Anforowicz, mlamouri+watch-blink_chromium.org, mlamouri+watch-content_chromium.org, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd UMA histograms to MHTML save operations.
In the context of the Offline project we need more information on how
the MHTML save operation is performing, especially in regards to how
much it is interfering with the render process main thread.
This change adds UMA histograms to track the performance of the costlier
operations performed when a page is being saved to MHTML. One new
"PageSerialization" category was added to encompass serialization generically
and a "MhtmlGeneration" sub-category for the MHTML specific parts.
All histograms added in this batch are timing measurements and they map closely
to the tracing instrumentation under the "page-serialization" category.
Following is the list of them with indentation to indicate what encompasses
what:
For the complete page saving operation:
PageSerialization.MhtmlGeneration.FullPageSavingTime
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.FrameTree
PageSerialization.MhtmlGeneration.RendererMainThreadTime.FrameTree
For a single frame in a page saving operation:
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.SingleFrame
PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame
PageSerialization.MhtmlGeneration.SerializationTime.SingleFrame
PageSerialization.SerializationTime.Html
*PageSerialization.SerializationTime.CSSElement
*PageSerialization.SerializationTime.ImageElement
PageSerialization.MhtmlGeneration.EncodingTime.SingleFrame
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame
*: may be executed many times under its parent.
BUG=645686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/0f5a7e209836a0d22f7ae69ff7729da2a1fccf3e
Cr-Commit-Position: refs/heads/master@{#421989}
Patch Set 1 #
Total comments: 2
Patch Set 2 : Fixed error causing MHTMLGenerationTest to fail. #
Total comments: 15
Patch Set 3 : Updated histogram names and descriptions; better time multiplier format. #Patch Set 4 : Ugly line breaks. #
Total comments: 4
Patch Set 5 : Rebase only. #Patch Set 6 : Reverted accidental histogram removal. #
Total comments: 8
Patch Set 7 : Address code review comments. #
Total comments: 6
Patch Set 8 : Address code review comments. #Patch Set 9 : Address code review comments. #Dependent Patchsets: Messages
Total messages: 53 (35 generated)
Description was changed from ========== Add UMA histograms to MHTML save operations. In the context of the Offline project we need more information on how the MHTML save operation is performing, especially in regards to how much it is interfering with the render process main thread. This change adds UMA histograms to track the performance of the costlier operations performed when a page is being saved to MHTML. I created two new categories for these metrics: MhtmlGeneration for code written specifically for MHTML saving, and PageSerialization for the more generic serialization pieces. Newly added histograms are: MhtmlGeneration.BrowserWaitTimeForFrameSerialization MhtmlGeneration.BrowserWaitTimeForFrameTreeSerialization MhtmlGeneration.RendererMainThreadTimeForFrameSerialization MhtmlGeneration.AllRenderersMainThreadTimeForFrameTreeSerialization MhtmlGeneration.WritingToDiskTime MhtmlGeneration.ExcessIpcToRendererProcessCount MhtmlGeneration.FullFrameSerializationTime MhtmlGeneration.FullFrameEncodingTime PageSerialization.HtmlSerializationTime PageSerialization.CSSSerializationTime PageSerialization.ImageSerializationTime All are timing measurements and they map closely to the tracing instrumentation under the "page-serialization" category. BUG=645686 ========== to ========== Add UMA histograms to MHTML save operations. In the context of the Offline project we need more information on how the MHTML save operation is performing, especially in regards to how much it is interfering with the render process main thread. This change adds UMA histograms to track the performance of the costlier operations performed when a page is being saved to MHTML. I created two new categories for these metrics: MhtmlGeneration for code written specifically for MHTML saving, and PageSerialization for the more generic serialization pieces. Newly added histograms are: MhtmlGeneration.BrowserWaitTimeForFrameSerialization MhtmlGeneration.BrowserWaitTimeForFrameTreeSerialization MhtmlGeneration.RendererMainThreadTimeForFrameSerialization MhtmlGeneration.AllRenderersMainThreadTimeForFrameTreeSerialization MhtmlGeneration.WritingToDiskTime MhtmlGeneration.ExcessIpcToRendererProcessCount MhtmlGeneration.FullFrameSerializationTime MhtmlGeneration.FullFrameEncodingTime PageSerialization.HtmlSerializationTime PageSerialization.CSSSerializationTime PageSerialization.ImageSerializationTime All are timing measurements and they map closely to the tracing instrumentation under the "page-serialization" category. BUG=645686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by carlosk@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...
Description was changed from ========== Add UMA histograms to MHTML save operations. In the context of the Offline project we need more information on how the MHTML save operation is performing, especially in regards to how much it is interfering with the render process main thread. This change adds UMA histograms to track the performance of the costlier operations performed when a page is being saved to MHTML. I created two new categories for these metrics: MhtmlGeneration for code written specifically for MHTML saving, and PageSerialization for the more generic serialization pieces. Newly added histograms are: MhtmlGeneration.BrowserWaitTimeForFrameSerialization MhtmlGeneration.BrowserWaitTimeForFrameTreeSerialization MhtmlGeneration.RendererMainThreadTimeForFrameSerialization MhtmlGeneration.AllRenderersMainThreadTimeForFrameTreeSerialization MhtmlGeneration.WritingToDiskTime MhtmlGeneration.ExcessIpcToRendererProcessCount MhtmlGeneration.FullFrameSerializationTime MhtmlGeneration.FullFrameEncodingTime PageSerialization.HtmlSerializationTime PageSerialization.CSSSerializationTime PageSerialization.ImageSerializationTime All are timing measurements and they map closely to the tracing instrumentation under the "page-serialization" category. BUG=645686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== Add UMA histograms to MHTML save operations. In the context of the Offline project we need more information on how the MHTML save operation is performing, especially in regards to how much it is interfering with the render process main thread. This change adds UMA histograms to track the performance of the costlier operations performed when a page is being saved to MHTML. I created two new categories for these metrics: MhtmlGeneration for code written specifically for MHTML saving, and PageSerialization for the more generic serialization pieces. Newly added histograms are: MhtmlGeneration.BrowserWaitTimeForFrameSerialization MhtmlGeneration.BrowserWaitTimeForFrameTreeSerialization MhtmlGeneration.RendererMainThreadTimeForFrameSerialization MhtmlGeneration.AllRenderersMainThreadTimeForFrameTreeSerialization MhtmlGeneration.WritingToDiskTime MhtmlGeneration.FullFrameSerializationTime MhtmlGeneration.FullFrameEncodingTime PageSerialization.HtmlSerializationTime PageSerialization.CSSSerializationTime PageSerialization.ImageSerializationTime All are timing measurements and they map closely to the tracing instrumentation under the "page-serialization" category. BUG=645686 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
carlosk@chromium.org changed reviewers: + dglazkov@chromium.org, isherman@chromium.org, nasko@chromium.org
carlosk@chromium.org changed reviewers: + dglazkov@chromium.org, isherman@chromium.org, nasko@chromium.org
https://codereview.chromium.org/2364923004/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2364923004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:288: if (renderer_main_thread_time > base::TimeDelta()) This is here to avoid adding potentially bogus values (never trust the renderer!). But should I do anything beyond just ignoring the value? https://codereview.chromium.org/2364923004/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2364923004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:5233: if (file.WriteAtCurrentPos(data.data(), data.size()) < 0) { I consolidated the file writing operations into a single block to more easily track their time. This is also a preparation for a follow up patch where I'll move file writing to another task to lessen the time we are blocking the renderer main thread.
https://codereview.chromium.org/2364923004/diff/1/content/browser/download/mh... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2364923004/diff/1/content/browser/download/mh... content/browser/download/mhtml_generation_manager.cc:288: if (renderer_main_thread_time > base::TimeDelta()) This is here to avoid adding potentially bogus values (never trust the renderer!). But should I do anything beyond just ignoring the value? https://codereview.chromium.org/2364923004/diff/1/content/renderer/render_fra... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2364923004/diff/1/content/renderer/render_fra... content/renderer/render_frame_impl.cc:5233: if (file.WriteAtCurrentPos(data.data(), data.size()) < 0) { I consolidated the file writing operations into a single block to more easily track their time. This is also a preparation for a follow up patch where I'll move file writing to another task to lessen the time we are blocking the renderer main thread.
nasko@: Please review changes in content/*. dglazkov@: Please review changes in third_party/WebKit/*. isherman@: Please review changes in histograms.xml. lukasza@: FYI.
The CQ bit was checked by carlosk@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...
https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right): https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:443: imageHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - imageStartTime) * 1000000)); nit: Are there any utility functions in Blink that provide more readable conversions among time units than just multiplying by a really large number? If not, it might be nice to add such -- it's hard to count the number of zeros here (and above, too, actually). https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25523: + serializing the frame tree of a page being saved to MHTML. Is this summed over renderer processes? How would someone go about interpreting this value, and distinguishing "one renderer is affected a lot" vs. "many are affected a bit"? Is that measured by some of the other histograms below? I think the answer is: It's measured by ""MhtmlGeneration.RendererMainThreadTimeForFrameSerialization". It would be nice if these histograms had more similar names. I'd suggest using more dots in the names -- something like "MhtmlGeneration.FrameSerialization.RendererMainThreadTime" and "MhtmlGeneration.FrameTreeSerialization.RendererMainThreadTime". Or some other scheme that makes sense to you -- just something that has more obvious parallels than the current names. https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25541: + respective render processes while saving a page into MHTML. Is this a sum over MhtmlGeneration.BrowserWaitTimeForFrameSerialization, or can the operations be done in parallel? If they can be done in parallel, what are the semantics of the wait time for just one frame? https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25545: +<histogram name="MhtmlGeneration.FullFrameEncodingTime" units="microseconds"> What is the importance of the word "full" in this name? Are partial frames also encoded? https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25561: +<histogram name="MhtmlGeneration.FullPageSavingTime" units="ms"> What's the difference between a "page" and "frame tree"? Are these the same concept? https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25580: +</histogram> Would it be appropriate to group all of these histograms under the PageSerialization category, e.g. as "PageSerialization.MhtmlGeneration.*"? https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40328: +</histogram> If there are multiple images on a page, presumably there will be multiple entries emitted to this histogram, right? Is there something that measures the overall time to serialize a full page? If not, is that something that would be useful to measure? (I think that the MhtmlGeneration histograms might measure this, but it's not totally clear to me what the relationship between the "PageSerialization" and the "MhtmlGeneration" histograms is.)
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 carlosk@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 unchecked by commit-bot@chromium.org
Dry run: 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...)
Description was changed from
==========
Add UMA histograms to MHTML save operations.
In the context of the Offline project we need more information on how
the MHTML save operation is performing, especially in regards to how
much it is interfering with the render process main thread.
This change adds UMA histograms to track the performance of the costlier
operations performed when a page is being saved to MHTML. I created two
new categories for these metrics: MhtmlGeneration for code written
specifically for MHTML saving, and PageSerialization for the more
generic serialization pieces.
Newly added histograms are:
MhtmlGeneration.BrowserWaitTimeForFrameSerialization
MhtmlGeneration.BrowserWaitTimeForFrameTreeSerialization
MhtmlGeneration.RendererMainThreadTimeForFrameSerialization
MhtmlGeneration.AllRenderersMainThreadTimeForFrameTreeSerialization
MhtmlGeneration.WritingToDiskTime
MhtmlGeneration.FullFrameSerializationTime
MhtmlGeneration.FullFrameEncodingTime
PageSerialization.HtmlSerializationTime
PageSerialization.CSSSerializationTime
PageSerialization.ImageSerializationTime
All are timing measurements and they map closely to the tracing
instrumentation under the "page-serialization" category.
BUG=645686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Add UMA histograms to MHTML save operations.
In the context of the Offline project we need more information on how
the MHTML save operation is performing, especially in regards to how
much it is interfering with the render process main thread.
This change adds UMA histograms to track the performance of the costlier
operations performed when a page is being saved to MHTML. One new
"PageSerialization" category was added to encompass serialization generically
and a "MhtmlGeneration" sub-category for the MHTML specific parts.
All histograms added in this batch are timing measurements and they map closely
to the tracing instrumentation under the "page-serialization" category.
Following is the list of them with indentation to indicate what encompasses
what:
For the complete page saving operation:
PageSerialization.MhtmlGeneration.FullPageSavingTime
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.FrameTree
PageSerialization.MhtmlGeneration.RendererMainThreadTime.FrameTree
For a single frame in a page saving operation:
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.SingleFrame
PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame
PageSerialization.MhtmlGeneration.SerializationTime.SingleFrame
PageSerialization.SerializationTime.Html
*PageSerialization.SerializationTime.CSSElement
*PageSerialization.SerializationTime.ImageElement
PageSerialization.MhtmlGeneration.EncodingTime.SingleFrame
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame
*: may be executed many times under its parent.
BUG=645686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
The CQ bit was checked by carlosk@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 Ilya. I did an overhaul of histogram names and descriptions and they do look better, I think. The only difference between these last two patch sets is the "ugly" line breaks for histogram names in C++ code. Reading the coding style I didn't find it was acceptable to leave them as they were in the previous patch (I didn't want to move them to the beginning of the file which is the only presented alternative in this case). https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right): https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:443: imageHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - imageStartTime) * 1000000)); On 2016/09/24 02:53:51, Ilya Sherman wrote: > nit: Are there any utility functions in Blink that provide more readable > conversions among time units than just multiplying by a really large number? If > not, it might be nice to add such -- it's hard to count the number of zeros here > (and above, too, actually). I'm not very used to Blink's codebase but I think the answer is no, there's no constants like that. At least that's what searching for the use pattern of these histograms shows me [1]. Hopefully at some point we will use the histogram macros from base/ now that the repos are merged. I agree that is annoying and I found a way to making it somewhat easier to read: using the scientific notation as it's the case in a few hits of the referenced codesearch. WDYT? [1] https://cs.chromium.org/search/?q="CustomCountHistogram" https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25523: + serializing the frame tree of a page being saved to MHTML. On 2016/09/24 02:53:52, Ilya Sherman wrote: > Is this summed over renderer processes? Yes, values are collected on the many renders used for saving one page, sent back to the browser which then adds all of them together and report to this metric. > How would someone go about interpreting > this value, and distinguishing "one renderer is affected a lot" vs. "many are > affected a bit"? Is that measured by some of the other histograms below? I > think the answer is: It's measured by > ""MhtmlGeneration.RendererMainThreadTimeForFrameSerialization". It would be > nice if these histograms had more similar names. I'd suggest using more dots in > the names -- something like > "MhtmlGeneration.FrameSerialization.RendererMainThreadTime" and > "MhtmlGeneration.FrameTreeSerialization.RendererMainThreadTime". Or some other > scheme that makes sense to you -- just something that has more obvious parallels > than the current names. Yes, that is the relation between those histograms. And I agree a better naming scheme would make the relationships clearer. I followed another logic for the naming though so let me know WDYT. https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25541: + respective render processes while saving a page into MHTML. On 2016/09/24 02:53:51, Ilya Sherman wrote: > Is this a sum over MhtmlGeneration.BrowserWaitTimeForFrameSerialization, or can > the operations be done in parallel? If they can be done in parallel, what are > the semantics of the wait time for just one frame? Yes, this is the sum of those for one save operation. They can't currently be done in parallel as they are written to file directly by the renderer so they must happen in order. This measurement is useful in contrast to MhtmlGeneration.AllRenderersMainThreadTimeForFrameTreeSerialization because it can show us how much IPC handling waiting time there is versus the time we actually occupy the renderer main thread. https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25545: +<histogram name="MhtmlGeneration.FullFrameEncodingTime" units="microseconds"> On 2016/09/24 02:53:51, Ilya Sherman wrote: > What is the importance of the word "full" in this name? Are partial frames also > encoded? This is in fact in consequence of the histogram below, MhtmlGeneration.FullFrameSerializationTime. I chose to add the "full" to try to clarify that this "serialization" includes all the frame's contents -- HTML and CSS + image resources -- in comparison to each of those things considered in isolation, which are reported in the other histograms. But indeed, it is not necessary if we'd like to keep names as concise as possible. https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25561: +<histogram name="MhtmlGeneration.FullPageSavingTime" units="ms"> On 2016/09/24 02:53:52, Ilya Sherman wrote: > What's the difference between a "page" and "frame tree"? Are these the same > concept? The "page" version includes a few more things when compared to the "frame tree", like opening and closing the MHTML file and a few more things. My adding the "full" again here now makes me think that's in fact confusing. I left it only here and removed it from the "frame" versions above. https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25580: +</histogram> On 2016/09/24 02:53:51, Ilya Sherman wrote: > Would it be appropriate to group all of these histograms under the > PageSerialization category, e.g. as "PageSerialization.MhtmlGeneration.*"? That would certainly make it clearer that these are all related. I don't appreciate all that much the much longer names they get but I think the former reason is more important. So done. https://codereview.chromium.org/2364923004/diff/20001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:40328: +</histogram> On 2016/09/24 02:53:52, Ilya Sherman wrote: > If there are multiple images on a page, presumably there will be multiple > entries emitted to this histogram, right? Is there something that measures the > overall time to serialize a full page? If not, is that something that would be > useful to measure? Correct, the Image and Css histograms may be created many times even for a single frame. I assume these metrics will be useful for a future change we envision to split the serialization work in more than one renderer main thread task. There are metrics that encompass these times you are referring to. Given the name changes I also updated the description to clarify "what encompasses what". > (I think that the MhtmlGeneration histograms might measure this, but it's not > totally clear to me what the relationship between the "PageSerialization" and > the "MhtmlGeneration" histograms is.) I tried to explain that in the description: | I created two | new categories for these metrics: MhtmlGeneration for code written | specifically for MHTML saving, and PageSerialization for the more | generic serialization pieces.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks, this is much clearer now =) LGTM https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right): https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:443: imageHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - imageStartTime) * 1000000)); On 2016/09/27 21:07:29, carlosk wrote: > On 2016/09/24 02:53:51, Ilya Sherman wrote: > > nit: Are there any utility functions in Blink that provide more readable > > conversions among time units than just multiplying by a really large number? > If > > not, it might be nice to add such -- it's hard to count the number of zeros > here > > (and above, too, actually). > > I'm not very used to Blink's codebase but I think the answer is no, there's no > constants like that. At least that's what searching for the use pattern of these > histograms shows me [1]. Hopefully at some point we will use the histogram > macros from base/ now that the repos are merged. > > I agree that is annoying and I found a way to making it somewhat easier to read: > using the scientific notation as it's the case in a few hits of the referenced > codesearch. WDYT? > > [1] https://cs.chromium.org/search/?q=%22CustomCountHistogram%22 Yep, it's a bit nicer this way -- thanks =) https://codereview.chromium.org/2364923004/diff/60001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2364923004/diff/60001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:268: renderer_wait_time); nit: One thing you could do to clean up the histogram names would be to define a local macro like #define RECORD_TIMING_HISTOGRAM(name, value) \ UMA_HISTOGRAM_TIMES("PageSerialization.MhtmlGeneration." ## name, value); and call this as RECORD_TIMING_HISTOGRAM("BrowserWaitForRendererTime.SingleFrame", renderer_wait_time); I don't promise that the syntax is right, but hopefully the idea is clear =) https://codereview.chromium.org/2364923004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2364923004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25523: -</histogram> nit: Please revert this removal -- I assume it's an accidental deletion, yeah?
Thanks. https://codereview.chromium.org/2364923004/diff/60001/content/browser/downloa... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2364923004/diff/60001/content/browser/downloa... content/browser/download/mhtml_generation_manager.cc:268: renderer_wait_time); On 2016/09/28 00:42:16, Ilya Sherman wrote: > nit: One thing you could do to clean up the histogram names would be to define a > local macro like > > #define RECORD_TIMING_HISTOGRAM(name, value) \ > UMA_HISTOGRAM_TIMES("PageSerialization.MhtmlGeneration." ## name, value); > > and call this as > > RECORD_TIMING_HISTOGRAM("BrowserWaitForRendererTime.SingleFrame", > renderer_wait_time); > > I don't promise that the syntax is right, but hopefully the idea is clear =) The idea is clear but it doesn't solve my issue of leaving the whole name in single string, what I find valuable when codesearch-ing for histograms. https://codereview.chromium.org/2364923004/diff/60001/tools/metrics/histogram... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/2364923004/diff/60001/tools/metrics/histogram... tools/metrics/histograms/histograms.xml:25523: -</histogram> On 2016/09/28 00:42:16, Ilya Sherman wrote: > nit: Please revert this removal -- I assume it's an accidental deletion, yeah? Gah! Thanks for noticing this one! My bad and reverted! Funny fact: for one moment I thought this was really removed in ToT as codesearch could not find any matches for Mist.SwitchResult. I have the impression that it used to index this file and show its contents but it's not the case anymore. :/
lukasza@chromium.org changed reviewers: + lukasza@chromium.org
LGTM with some nits and suggestions. https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:47: const base::TimeTicks creation_time() { return creation_time_; } The |const| in |const T field() { return field_)| above looks weird. Did you mean to use 1) const T& field() ... and/or 2) T field() const { ... } ? https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:269: all_renderers_wait_time_ += renderer_wait_time; Would it make sense to null-out/zero-out |wait_on_renderer_start_time_| here? I think there were cases when MarkAsFinished could be called twice (in some kind of a race between 1) a real job completion IPC and 2) renderer exit notification). I think these races are fixed, but zeroing-out the value should improve resiliency of the new code to such duplicate completion notifications. https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:280: all_renderers_main_thread_time_); Same suggestion (zeroing out the fields) as above here and in the other places. https://codereview.chromium.org/2364923004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2364923004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:5247: for (WebData& data : mhtml_contents) { nit: const WebData&
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Thanks. https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... File content/browser/download/mhtml_generation_manager.cc (right): https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:47: const base::TimeTicks creation_time() { return creation_time_; } On 2016/09/28 19:38:40, Łukasz Anforowicz wrote: > The |const| in |const T field() { return field_)| above looks weird. Did you > mean to use > > 1) const T& field() ... > > and/or > > 2) T field() const { ... } > > ? Done #2. https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:269: all_renderers_wait_time_ += renderer_wait_time; On 2016/09/28 19:38:40, Łukasz Anforowicz wrote: > Would it make sense to null-out/zero-out |wait_on_renderer_start_time_| here? I > think there were cases when MarkAsFinished could be called twice (in some kind > of a race between 1) a real job completion IPC and 2) renderer exit > notification). I think these races are fixed, but zeroing-out the value should > improve resiliency of the new code to such duplicate completion notifications. Done, but checking |is_finished_| instead. https://codereview.chromium.org/2364923004/diff/100001/content/browser/downlo... content/browser/download/mhtml_generation_manager.cc:280: all_renderers_main_thread_time_); On 2016/09/28 19:38:40, Łukasz Anforowicz wrote: > Same suggestion (zeroing out the fields) as above here and in the other places. Ditto. https://codereview.chromium.org/2364923004/diff/100001/content/renderer/rende... File content/renderer/render_frame_impl.cc (right): https://codereview.chromium.org/2364923004/diff/100001/content/renderer/rende... content/renderer/render_frame_impl.cc:5247: for (WebData& data : mhtml_contents) { On 2016/09/28 19:38:40, Łukasz Anforowicz wrote: > nit: const WebData& Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
IPC LGTM, content/ rubberstamp based on lukasza@'s review.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right): https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:323: cssStartTime = WTF::monotonicallyIncreasingTime(); shouldn't need WTF:: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/CurrentTim... https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:356: cssHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - cssStartTime) * 1e6)); What are these arbitrary-looking multipliers (1e6, 1e7)? https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:443: imageHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - imageStartTime) * 1e6)); Same here re: WTF.
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
Thanks. https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right): https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:323: cssStartTime = WTF::monotonicallyIncreasingTime(); On 2016/09/29 04:02:55, dglazkov wrote: > shouldn't need WTF:: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/CurrentTim... Done. https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:356: cssHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - cssStartTime) * 1e6)); On 2016/09/29 04:02:55, dglazkov wrote: > What are these arbitrary-looking multipliers (1e6, 1e7)? They make it a bit easier to understand what is the time multiplier. See my previous discussion with Ilya: https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/core/frame/FrameSerializer.cpp:443: imageHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - imageStartTime) * 1e6)); On 2016/09/29 04:02:55, dglazkov wrote: > Same here re: WTF. Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
On 2016/09/29 at 18:24:11, carlosk wrote: > Thanks. > > https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/core/frame/FrameSerializer.cpp (right): > > https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameSerializer.cpp:323: cssStartTime = WTF::monotonicallyIncreasingTime(); > On 2016/09/29 04:02:55, dglazkov wrote: > > shouldn't need WTF:: > > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/wtf/CurrentTim... > > Done. > > https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/core/frame/FrameSerializer.cpp:356: cssHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - cssStartTime) * 1e6)); > On 2016/09/29 04:02:55, dglazkov wrote: > > What are these arbitrary-looking multipliers (1e6, 1e7)? > > They make it a bit easier to understand what is the time multiplier. See my previous discussion with Ilya: https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... Ah, got it. Let's make it one step easier to understand: 1) define a constant that describes the upper bound. For example, "const int32_t maxSomethingDurationUmaMs" 2) specify this constant as a multiplication of logical parts. For example, "const int32_t name = 60 * 60 * 1000" 3) use the constant in the code. Here's a nice example here: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/Auto... Applying this to all arbitrary-looking numbers makes life saner for future us looking at this code :)
The CQ bit was checked by carlosk@chromium.org to run a CQ dry run
On 2016/09/29 18:41:33, dglazkov wrote: > On 2016/09/29 at 18:24:11, carlosk wrote: > > > https://codereview.chromium.org/2364923004/diff/120001/third_party/WebKit/Sou... > > third_party/WebKit/Source/core/frame/FrameSerializer.cpp:356: > cssHistogram.count(static_cast<int64_t>((WTF::monotonicallyIncreasingTime() - > cssStartTime) * 1e6)); > > On 2016/09/29 04:02:55, dglazkov wrote: > > > What are these arbitrary-looking multipliers (1e6, 1e7)? > > > > They make it a bit easier to understand what is the time multiplier. See my > previous discussion with Ilya: > https://codereview.chromium.org/2364923004/diff/20001/third_party/WebKit/Sour... > > Ah, got it. Let's make it one step easier to understand: > > 1) define a constant that describes the upper bound. For example, "const int32_t > maxSomethingDurationUmaMs" > 2) specify this constant as a multiplication of logical parts. For example, > "const int32_t name = 60 * 60 * 1000" > 3) use the constant in the code. > > Here's a nice example here: > https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/html/Auto... > > Applying this to all arbitrary-looking numbers makes life saner for future us > looking at this code :) Done.
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
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 carlosk@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from isherman@chromium.org, lukasza@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2364923004/#ps160001 (title: "Address code review comments.")
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 #9 (id:160001)
Message was sent while issue was closed.
Description was changed from
==========
Add UMA histograms to MHTML save operations.
In the context of the Offline project we need more information on how
the MHTML save operation is performing, especially in regards to how
much it is interfering with the render process main thread.
This change adds UMA histograms to track the performance of the costlier
operations performed when a page is being saved to MHTML. One new
"PageSerialization" category was added to encompass serialization generically
and a "MhtmlGeneration" sub-category for the MHTML specific parts.
All histograms added in this batch are timing measurements and they map closely
to the tracing instrumentation under the "page-serialization" category.
Following is the list of them with indentation to indicate what encompasses
what:
For the complete page saving operation:
PageSerialization.MhtmlGeneration.FullPageSavingTime
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.FrameTree
PageSerialization.MhtmlGeneration.RendererMainThreadTime.FrameTree
For a single frame in a page saving operation:
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.SingleFrame
PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame
PageSerialization.MhtmlGeneration.SerializationTime.SingleFrame
PageSerialization.SerializationTime.Html
*PageSerialization.SerializationTime.CSSElement
*PageSerialization.SerializationTime.ImageElement
PageSerialization.MhtmlGeneration.EncodingTime.SingleFrame
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame
*: may be executed many times under its parent.
BUG=645686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
==========
to
==========
Add UMA histograms to MHTML save operations.
In the context of the Offline project we need more information on how
the MHTML save operation is performing, especially in regards to how
much it is interfering with the render process main thread.
This change adds UMA histograms to track the performance of the costlier
operations performed when a page is being saved to MHTML. One new
"PageSerialization" category was added to encompass serialization generically
and a "MhtmlGeneration" sub-category for the MHTML specific parts.
All histograms added in this batch are timing measurements and they map closely
to the tracing instrumentation under the "page-serialization" category.
Following is the list of them with indentation to indicate what encompasses
what:
For the complete page saving operation:
PageSerialization.MhtmlGeneration.FullPageSavingTime
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.FrameTree
PageSerialization.MhtmlGeneration.RendererMainThreadTime.FrameTree
For a single frame in a page saving operation:
PageSerialization.MhtmlGeneration.BrowserWaitForRendererTime.SingleFrame
PageSerialization.MhtmlGeneration.RendererMainThreadTime.SingleFrame
PageSerialization.MhtmlGeneration.SerializationTime.SingleFrame
PageSerialization.SerializationTime.Html
*PageSerialization.SerializationTime.CSSElement
*PageSerialization.SerializationTime.ImageElement
PageSerialization.MhtmlGeneration.EncodingTime.SingleFrame
PageSerialization.MhtmlGeneration.WriteToDiskTime.SingleFrame
*: may be executed many times under its parent.
BUG=645686
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/0f5a7e209836a0d22f7ae69ff7729da2a1fccf3e
Cr-Commit-Position: refs/heads/master@{#421989}
==========
Message was sent while issue was closed.
Patchset 9 (id:??) landed as https://crrev.com/0f5a7e209836a0d22f7ae69ff7729da2a1fccf3e Cr-Commit-Position: refs/heads/master@{#421989} |
