Track performance of toBlob and its complete timeout delay (issue 2039673002 by xidachen@chromium.org)

[xida...@chromium.org]

Reviewers: xlai (Olivia), Justin Novosad, Ilya Sherman
CL: https://codereview.chromium.org/2039673002/

Message:
xlai@: could you please make sure that I place the enum histogram and the timer
in the right place?

Description:
Track performance of toBlob and its complete timeout delay

This CL does two things:
1. Track the performance of a toBlob API call. Because toBlob has a call
back, so we put a timer in the CanvasAsyncToBlob class.
2. Track how often the complete timeout delay happens in toBlob calls.

BUG=612585, 608815

Base URL: https://chromium.googlesource.com/chromium/src.git@master

Affected files (+53, -0 lines):
M third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
M third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
M tools/metrics/histograms/histograms.xml


Index: third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
diff --git a/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp b/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
index 01136649524082b8b79b83dda662a2a4852c957e..32aa475cf162aa0828a5c3652916d23f8d0f32f8 100644
--- a/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
+++ b/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
@@ -5,6 +5,7 @@
#include "core/html/canvas/CanvasAsyncBlobCreator.h"

#include "core/fileapi/Blob.h"
+#include "platform/Histogram.h"
#include "platform/ThreadSafeFunctional.h"
#include "platform/graphics/ImageBuffer.h"
#include "platform/image-encoders/JPEGImageEncoder.h"
@@ -94,6 +95,7 @@ CanvasAsyncBlobCreator::CanvasAsyncBlobCreator(DOMUint8ClampedArray* data, MimeT
m_pixelRowStride = size.width() * NumChannelsPng;
m_idleTaskStatus = IdleTaskNotSupported;
m_numRowsCompleted = 0;
+ m_startTime = WTF::monotonicallyIncreasingTime();
}

CanvasAsyncBlobCreator::~CanvasAsyncBlobCreator()
@@ -251,6 +253,24 @@ void CanvasAsyncBlobCreator::encodeRowsJpegOnMainThread()
this->signalAlternativeCodePathFinishedForTesting();
}

+void CanvasAsyncBlobCreator::recordElapsedTime()
+{
+ double elapsedTime = WTF::monotonicallyIncreasingTime() - m_startTime;
+ if (m_mimeType == MimeTypePng) {
+ DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, toBlobPNGCounter, new CustomCountHistogram("Blink.Canvas.ToBlob.PNG", 0, 10000000, 50));
+ toBlobPNGCounter.count(elapsedTime * 1000000.0);
+ } else if (m_mimeType == MimeTypeJpeg) {
+ DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, toBlobJPEGCounter, new CustomCountHistogram("Blink.Canvas.ToBlob.JPEG", 0, 10000000, 50));
+ toBlobJPEGCounter.count(elapsedTime * 1000000.0);
+ } else if (m_mimeType == MimeTypeWebp) {
+ DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, toBlobWEBPCounter, new CustomCountHistogram("Blink.Canvas.ToBlob.WEBP", 0, 10000000, 50));
+ toBlobWEBPCounter.count(elapsedTime * 1000000.0);
+ } else {
+ DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, toBlobUnknownCounter, new CustomCountHistogram("Blink.Canvas.ToBlob.Unknown", 0, 10000000, 50));
+ toBlobUnknownCounter.count(elapsedTime * 1000000.0);
+ }
+}
+
void CanvasAsyncBlobCreator::createBlobAndInvokeCallback()
{
ASSERT(isMainThread());
@@ -260,6 +280,7 @@ void CanvasAsyncBlobCreator::createBlobAndInvokeCallback()
// non-GC members to allow teardown of CanvasAsyncBlobCreator.
m_data.clear();
m_callback.clear();
+ recordElapsedTime();
}

void CanvasAsyncBlobCreator::createNullAndInvokeCallback()
@@ -270,6 +291,7 @@ void CanvasAsyncBlobCreator::createNullAndInvokeCallback()
// clear non-GC members to allow teardown of CanvasAsyncBlobCreator.
m_data.clear();
m_callback.clear();
+ recordElapsedTime();
}

void CanvasAsyncBlobCreator::encodeImageOnEncoderThread(double quality)
@@ -344,6 +366,9 @@ void CanvasAsyncBlobCreator::idleTaskCompleteTimeoutEvent()
{
ASSERT(m_idleTaskStatus != IdleTaskNotStarted);

+ DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram, idleTaskCompleteTimeoutDelayHistogram, new EnumerationHistogram("Canvas.ToBlob.IdleTaskCompleteTimeoutDelay", NumberOfMimeTypeSupported));
+ idleTaskCompleteTimeoutDelayHistogram.count(m_mimeType);
+
if (m_idleTaskStatus == IdleTaskStarted) {
// It has taken too long to complete for the idle task.
m_idleTaskStatus = IdleTaskSwitchedToMainThreadTask;
Index: third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
diff --git a/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h b/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
index 64152fb3c0a5e14841d3073b9220150604079942..4bd048ec667318710304fde5672e8ae961ca8cb5 100644
--- a/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
+++ b/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
@@ -73,6 +73,8 @@ private:
size_t m_pixelRowStride;
const MimeType m_mimeType;
CrossThreadPersistent<BlobCallback> m_callback;
+ double m_startTime;
+ void recordElapsedTime();

// PNG
bool initializePngStruct();
Index: tools/metrics/histograms/histograms.xml
diff --git a/tools/metrics/histograms/histograms.xml b/tools/metrics/histograms/histograms.xml
index eeea9193acb9d4e9a4283bcdc7b6759c2d6d35be..42f37a99172d2f420bb70bec2ef81b07c01149b3 100644
--- a/tools/metrics/histograms/histograms.xml
+++ b/tools/metrics/histograms/histograms.xml
@@ -3478,6 +3478,11 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
<summary>Time spent on 2D canvas putImageData API call.</summary>
</histogram>

+<histogram name="Blink.Canvas.ToBlob" units="microseconds">
+ <owner>ju...@chromium.org</owner&gt;
+ <summary>Time spent on 2D canvas toBlob API call.</summary>
+</histogram>
+
<histogram name="Blink.Canvas.ToDataURL" units="microseconds">
<owner>ju...@chromium.org</owner&gt;
<summary>Time spent on 2D canvas toDataURL API call.</summary>
@@ -4336,6 +4341,13 @@ http://cs/file:chrome/histograms.xml - but prefer this file for new entries.
</summary>
</histogram>

+<histogram name="Canvas.ToBlob.IdleTaskCompleteTimeoutDelay" enum="MimeType">
+ <owner>xl...@chromium.org</owner&gt;
+ <summary>
+ Records the occurence of complete time out delay in canvas toBlob calls.
+ </summary>
+</histogram>
+
<histogram name="CAPSUpdater.Step" enum="CAPSUpdaterStep">
<owner>c...@chromium.org</owner&gt;
<summary>
@@ -80730,6 +80742,12 @@ To add a new entry, add it with any value and run test to compute valid value.
<int value="2" label="IPsec"/>
</enum>

+<enum name="MimeType" type="int">
+ <int value="0" label="Png"/>
+ <int value="1" label="Jpeg"/>
+ <int value="2" label="Webp"/>
+</enum>
+
<enum name="MissingStartType" type="int">
<int value="0" label="Nothing missing"/>
<int value="1" label="Start missing"/>
@@ -91812,6 +91830,14 @@ To add a new entry, add it with any value and run test to compute valid value.
<affected-histogram name="Blink.Canvas.PutImageData"/>
</histogram_suffixes>

+<histogram_suffixes name="BlinkCanvasToBlobTime" separator=".">
+ <suffix name="JPEG"/>
+ <suffix name="PNG"/>
+ <suffix name="Unknown"/>
+ <suffix name="WEBP"/>
+ <affected-histogram name="Blink.Canvas.ToBlob"/>
+</histogram_suffixes>
+
<histogram_suffixes name="BlinkCanvasToDataURLTime" separator=".">
<suffix name="BMP"/>
<suffix name="GIF"/>

[ju...@chromium.org]

https://codereview.chromium.org/2039673002/diff/20001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/20001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode294
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:294:
recordElapsedTime();
The failure case should not be recording to the same timing metrics as
the success case. We could probably do without capturing time for the
failure cases. However, it would be relevant to have a fail vs. succeed
histogram for each mime type.

https://codereview.chromium.org/2039673002/diff/20001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode369
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:369:

DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
idleTaskCompleteTimeoutDelayHistogram, new
EnumerationHistogram("Canvas.ToBlob.IdleTaskCompleteTimeoutDelay",
NumberOfMimeTypeSupported));

This histogram does not provide very significant info on its own. It
would be better to have a separate histogram per mime type, and for each
such histogram, the bins should be idleTaskTimeout vs.
idleTaskCompleted. That way we get histograms that tell us the fraction
of tasks that timed-out, which is directly relevant data. Also, the
word "Delay" is not very relevant here.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

PTAL

Right now we are measuring 3 things:
1. Duration of a toBlob API call.

2. Frequency of occurrence of start & complete timeout event. 4 histograms (PNG,
JPEG, WEBP, Unknown), and each one has 2 bins (start, complete).

3. Duration of each timeout event. Similar to the above, 4 histograms and each
has 2 bins.

https://codereview.chromium.org/2039673002/

[xl...@chromium.org]

Hi Xida, as a summary, for the three things that you're measuring:

1. Duration of a toBlob API call

--> Please remove measuring the duration of a failed call for different mime
type; we might be interested to know that something fails but we're not
interested to know how long it fails for different mine type.
--> Also the beginning of the duration measurement is not right.

2. Frequency of occurrence of start & complete timeout event. 4 histograms (PNG,
JPEG, WEBP, Unknown), and each one has 2 bins (start, complete).

--> I think the histogram you're adding for this part is unnecessary. For *ALL*
toBlob calls that have idle task supported, start timeout event is always posted
and complete timeout event is also often posted. What's the purpose of recording
that these two timeout events happen?
--> In fact, I think a more meaningful measurement should be recording down when
a "switch" from idle to non-idle happens. And I believe that's what Issue 608815
intends to do.

3. Duration of each timeout event. Similar to the above, 4 histograms and each
has 2 bins.

--> I don't think we need to have "unknown" bin. It was ASSERT_NOT_REACHED()
from the very beginning.



https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode98
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:98:
m_startTime = WTF::monotonicallyIncreasingTime();
To measure the total elapsed time of toBlob API call, I find it odd to
start the
measurement from here. The CanvasAsyncBlobCreator is created in the
*middle* of
the toBlob API call. You should start the timer from the beginning of
HTMLCanvasElement::toBlob().

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode277
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:277:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram,
toBlobUnknownCounter, new
CustomCountHistogram("Blink.Canvas.ToBlob.Unknown.Succeed", 0, 10000000,
50));
"Unknown" type is impossible to reach here. See "ASSERT_NOT_REACH()" in
above code.

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode292
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:292:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, toBlobPNGCounter,
new CustomCountHistogram("Blink.Canvas.ToBlob.PNG.Failed", 0, 10000000,
50));
I sincerely feels that such a detailed histogram for "Failed" use case
is unnecessary. I don't really think we're interested to know how long
it takes to fail.

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode342
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:342:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
idleTaskTimeoutEventPNG, new
EnumerationHistogram("Canvas.ToBlob.IdleTaskTimeout.PNG",
IdleTaskTimeoutSupported));
What is the purpose of this histogram?

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode370
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:370:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram,
toBlobUnknownStartTimeoutCounter, new
CustomCountHistogram("Blink.Canvas.ToBlobTimeoutDuration.Unknown.Start",
0, 10000000, 50));
Same comment for "unknown".

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

xlai@: I believe I have addressed all comments. PTAL

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode98
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:98:
m_startTime = WTF::monotonicallyIncreasingTime();

On 2016/06/07 16:08:14, xlai (Olivia) wrote:
> To measure the total elapsed time of toBlob API call, I find it odd to
start the
> measurement from here. The CanvasAsyncBlobCreator is created in the
*middle* of
> the toBlob API call. You should start the timer from the beginning of
> HTMLCanvasElement::toBlob().

Yes, that makes total sense. I place the start after checking
originClean() and isPaintable().

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode277
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:277:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram,
toBlobUnknownCounter, new
CustomCountHistogram("Blink.Canvas.ToBlob.Unknown.Succeed", 0, 10000000,
50));

On 2016/06/07 16:08:14, xlai (Olivia) wrote:
> "Unknown" type is impossible to reach here. See "ASSERT_NOT_REACH()"
in above
> code.

Done.

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode292
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:292:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram, toBlobPNGCounter,
new CustomCountHistogram("Blink.Canvas.ToBlob.PNG.Failed", 0, 10000000,
50));

On 2016/06/07 16:08:14, xlai (Olivia) wrote:
> I sincerely feels that such a detailed histogram for "Failed" use case
is
> unnecessary. I don't really think we're interested to know how long it
takes to
> fail.

Done.

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode342
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:342:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
idleTaskTimeoutEventPNG, new
EnumerationHistogram("Canvas.ToBlob.IdleTaskTimeout.PNG",
IdleTaskTimeoutSupported));

On 2016/06/07 16:08:14, xlai (Olivia) wrote:
> What is the purpose of this histogram?

Change this to record the occurrence of switch from idle to main thread.

https://codereview.chromium.org/2039673002/diff/120001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode370
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:370:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram,
toBlobUnknownStartTimeoutCounter, new
CustomCountHistogram("Blink.Canvas.ToBlobTimeoutDuration.Unknown.Start",
0, 10000000, 50));

On 2016/06/07 16:08:14, xlai (Olivia) wrote:
> Same comment for "unknown".

Done.

https://codereview.chromium.org/2039673002/diff/160001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/160001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode345
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:345:
}
Is this recording meaningful now that we change to record how many times
it switch from idle to main thread?

https://codereview.chromium.org/2039673002/

[ju...@chromium.org]

https://codereview.chromium.org/2039673002/diff/120001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/120001/tools/metrics/histograms/histograms.xml#newcode3482
tools/metrics/histograms/histograms.xml:3482: +
<owner>ju...@chromium.org</owner&gt;
You can put more that one owner, you know? Please put xlai and myself on
all of these histograms.

https://codereview.chromium.org/2039673002/diff/160001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2039673002/diff/160001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode39
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:39:
enum IdleTaskTimeoutType {
I find these enum names a bit hard to interpret. We have basically four
categories of outcome: Failed, completed before idle task timeout,
completed after idle task timeout, completed without using idle
scheduling (for webp). each call to toBlob should add a count to exactly
one of these bins. "Failed" could be expanded into more bins if we want
to identify failure types.

https://codereview.chromium.org/2039673002/

[ishe...@chromium.org]

I've been hanging back while you figure out exactly what histograms you'd like
to log, but I do have one quick comment right now:


https://codereview.chromium.org/2039673002/diff/160001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/160001/tools/metrics/histograms/histograms.xml#newcode91905
tools/metrics/histograms/histograms.xml:91905: + <suffix
name="Complete"/>
I don't understand what it means to start vs. complete a timeout. If
this is an important distinction to measure, could you please clarify
within the description text in this file what each of these cases means?

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

PTAL.

So we are recording a few things:
1. The time spent on an entire toBlob API calls.
2. Elapsed time starting from scheduleInitiateEncoding to the actual start of
initiateEncoding.
3. Elapsed time from start of idleEncode to the point when idle encode is done.
4. Types of toBlob outcome: Failed, IdleNotSupported (webp), SwitchedToMain,
Completed (without switching to main).

https://codereview.chromium.org/2039673002/

[xl...@chromium.org]

Besides the in-line comments below, I have two high-level comments:

1. I don't think you need to use thread safe histogram. Although webp encoding
is carried out on non-main thread (which is the only non-main-thread operation
in
the entire toBlob thing), the recording point (i.e createBlob or createNull) is
always happening on main thread.

2. I have a feeling that the enum histogram is not coded correctly here. It
seems to me that the patch mistakes between "suffix" and enum type themselves.
Could you double check with the documentation in
//src/tools/metrics/histograms/histograms.xml?


https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode207
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:207:
toBlobPNGIdleEncodeCounter.count(elapsedTime * 1000000.0);
Move this whole block of code before this if-else block.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode227
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:227:
toBlobJPEGIdleEncodeCounter.count(elapsedTime * 1000000.0);
Move this whole block of code before this if-else block.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode277
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:277:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
toBlobIdleSwitchedToMain, new
EnumerationHistogram("Blink.Canvas.ToBlob.IdleTask.SwitchedToMain",
IdleTaskNotSupported));
The enum type "SwitchedToMain" is misleading. We are always on main
thread. Change the name to "IdleTaskSwitchedToMainThreadTask". Also
change the other names to look exactly same as the enum types in
m_idleTaskStatus.

https://codereview.chromium.org/2039673002/

[ju...@chromium.org]

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode206
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:206:
DEFINE_THREAD_SAFE_STATIC_LOCAL(CustomCountHistogram,
toBlobPNGIdleEncodeCounter, new
CustomCountHistogram("Blink.Canvas.ToBlob.IdleEncodeDuration.PNG", 0,
10000000, 50));
This stat is not very useful the way it is currently computed. What we
call an "encode duration" should probably be the *sum* of the time spent
in this function for a given encode task.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode207
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:207:
toBlobPNGIdleEncodeCounter.count(elapsedTime * 1000000.0);

On 2016/06/13 20:15:06, xlai (Olivia) wrote:
> Move this whole block of code before this if-else block.

actually, I was think more like doing "m_elapsedTime += ..." in the "if"
part, and recording the histogram in the "else" part. Same thing for
JPEG.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode277
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:277:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
toBlobIdleSwitchedToMain, new
EnumerationHistogram("Blink.Canvas.ToBlob.IdleTask.SwitchedToMain",
IdleTaskNotSupported));

On 2016/06/13 20:15:06, xlai (Olivia) wrote:
> The enum type "SwitchedToMain" is misleading. We are always on main
thread.
> Change the name to "IdleTaskSwitchedToMainThreadTask". Also change the
other
> names to look exactly same as the enum types in m_idleTaskStatus.

Also, in the case where we switch to main, there are two interesting
timing stats to record: the time spent encoding before and after the
switch.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode207
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:207:
toBlobPNGIdleEncodeCounter.count(elapsedTime * 1000000.0);

On 2016/06/13 21:21:35, Justin Novosad wrote:
> On 2016/06/13 20:15:06, xlai (Olivia) wrote:
> > Move this whole block of code before this if-else block.
>
> actually, I was think more like doing "m_elapsedTime += ..." in the
"if" part,
> and recording the histogram in the "else" part. Same thing for JPEG.

Yes, that makes perfect sense.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode227
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:227:
toBlobJPEGIdleEncodeCounter.count(elapsedTime * 1000000.0);

On 2016/06/13 20:15:06, xlai (Olivia) wrote:
> Move this whole block of code before this if-else block.

Using Justin's suggestion.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode277
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:277:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
toBlobIdleSwitchedToMain, new
EnumerationHistogram("Blink.Canvas.ToBlob.IdleTask.SwitchedToMain",
IdleTaskNotSupported));
On 2016/06/13 20:15:06, xlai (Olivia) wrote:
> The enum type "SwitchedToMain" is misleading. We are always on main
thread.
> Change the name to "IdleTaskSwitchedToMainThreadTask". Also change the
other
> names to look exactly same as the enum types in m_idleTaskStatus.

Done.

https://codereview.chromium.org/2039673002/diff/220001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode277
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:277:
DEFINE_THREAD_SAFE_STATIC_LOCAL(EnumerationHistogram,
toBlobIdleSwitchedToMain, new
EnumerationHistogram("Blink.Canvas.ToBlob.IdleTask.SwitchedToMain",
IdleTaskNotSupported));

On 2016/06/13 21:21:35, Justin Novosad wrote:
> On 2016/06/13 20:15:06, xlai (Olivia) wrote:
> > The enum type "SwitchedToMain" is misleading. We are always on main
thread.
> > Change the name to "IdleTaskSwitchedToMainThreadTask". Also change
the other
> > names to look exactly same as the enum types in m_idleTaskStatus.
>
> Also, in the case where we switch to main, there are two interesting
timing
> stats to record: the time spent encoding before and after the switch.

This actually brings up another question: currently we measure the time
spent encoding on the idle period. That time could be encoding the
entire image, it could also be encoding part of the image because the
switch could happen. In other words, that timing may not be so
meaningful?

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode282
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:282:
DEFINE_STATIC_LOCAL(EnumerationHistogram, toBlobIdleTaskStatus,
("Blink.Canvas.ToBlob.IdleTaskStatus", IdleTaskNotSupported));
xlai@: you are correct. I change the enum histogram like this. But one
problem is that we cannot measure the case when toBlob failed because
that happens in createNullAndInvokeCallback().

https://codereview.chromium.org/2039673002/

[ju...@chromium.org]

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode214
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:214:
m_elapsedTime += (WTF::monotonicallyIncreasingTime() -
m_initiateStartTime);
At each Idle slice, you are adding the time since start? This is not
correct.

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode234
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:234:
m_elapsedTime += (WTF::monotonicallyIncreasingTime() -
m_initiateStartTime);
Not correct.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/240001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode214
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:214:
m_elapsedTime += (WTF::monotonicallyIncreasingTime() -
m_initiateStartTime);

On 2016/06/22 14:38:57, Justin Novosad wrote:
> At each Idle slice, you are adding the time since start? This is not
correct.

The new patch should have that fixed.

https://codereview.chromium.org/2039673002/

[ju...@chromium.org]

I think this works. lgtm
Please wait for Olivia's stamp of approval.

https://codereview.chromium.org/2039673002/

[xl...@chromium.org]

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode213
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:213:
toBlobPNGIdleEncodeCounter.count(m_elapsedTime * 1000000.0);
I think there is a logic flaw. Think of the scenario when in the last
idleEncodeRowsPng() call, the encoding finishes the last few rows of
encoding without hitting the deadline in that particular idle period, so
the above addition to m_elapsedTime at Line 201 is not executed. Then in
here, the recording will miss out the time spent in the last
idleEncodeRowsPng() call.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode244
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:244:
m_elapsedTime += (WTF::monotonicallyIncreasingTime() - startTime);
There is also a logic flaw here similar to the PNG one but the solution
may be different. The function
JPEGImageEncoder::progressiveEncodeRowsJpegHelper() will stop when it
uses up the deadlineSeconds passed into it. If it manages to complete
all encoding, you'll never hit the Line 244 and so your m_elapsedTime
does not record the time spent in encoding. My suggestion for this one
would be to move Line 244 to the position between Line 230 and Line 231.
In that way, you accumulate m_elapsedTime regardless of failed,
complete, in-progress status.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode283
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:283:
toBlobIdleTaskStatus.count(m_idleTaskStatus);
You miss out the case when m_idleTaskStatus==IdleTaskFailed because
createBlobAndInvokeCallback() is only executed when idle encoding
succeeds.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode82
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:82:
double m_elapsedTime = 0;
Nit: move initialization of this member to the initialization list of
constructor to be consistent with other members.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode213
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:213:
toBlobPNGIdleEncodeCounter.count(m_elapsedTime * 1000000.0);

On 2016/06/23 17:07:37, xlai (Olivia) wrote:
> I think there is a logic flaw. Think of the scenario when in the last
> idleEncodeRowsPng() call, the encoding finishes the last few rows of
encoding
> without hitting the deadline in that particular idle period, so the
above
> addition to m_elapsedTime at Line 201 is not executed. Then in here,
the
> recording will miss out the time spent in the last idleEncodeRowsPng()
call.

Good catch, the new patch should have that fixed.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode244
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:244:
m_elapsedTime += (WTF::monotonicallyIncreasingTime() - startTime);

On 2016/06/23 17:07:37, xlai (Olivia) wrote:
> There is also a logic flaw here similar to the PNG one but the
solution may be
> different. The function
JPEGImageEncoder::progressiveEncodeRowsJpegHelper() will
> stop when it uses up the deadlineSeconds passed into it. If it manages
to
> complete all encoding, you'll never hit the Line 244 and so your
m_elapsedTime
> does not record the time spent in encoding. My suggestion for this one
would be
> to move Line 244 to the position between Line 230 and Line 231. In
that way, you
> accumulate m_elapsedTime regardless of failed, complete, in-progress
status.

Done.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode283
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:283:
toBlobIdleTaskStatus.count(m_idleTaskStatus);

On 2016/06/23 17:07:37, xlai (Olivia) wrote:
> You miss out the case when m_idleTaskStatus==IdleTaskFailed because
> createBlobAndInvokeCallback() is only executed when idle encoding
succeeds.

Done.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode82
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:82:
double m_elapsedTime = 0;

On 2016/06/23 17:07:37, xlai (Olivia) wrote:
> Nit: move initialization of this member to the initialization list of
> constructor to be consistent with other members.

Done.

https://codereview.chromium.org/2039673002/

[xl...@chromium.org]

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode196
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:196:
double startTime = WTF::monotonicallyIncreasingTime();
put this back.

https://codereview.chromium.org/2039673002/diff/260001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode201
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:201:

m_elapsedTime += (WTF::monotonicallyIncreasingTime() - startTime);

Put this back.

https://codereview.chromium.org/2039673002/diff/280001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/280001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode199
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:199:
double startTime = WTF::monotonicallyIncreasingTime();
This solution does not look great. Recording the time at every row
encoding adds up the duration unnecessarily. I would suggest another
approach that solves this issue but won't cause this (pls follow
instructions on other comments).
First of all, remove this line.

https://codereview.chromium.org/2039673002/diff/280001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode207
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:207:

m_elapsedTime += (WTF::monotonicallyIncreasingTime() - startTime);

Remove this.

https://codereview.chromium.org/2039673002/diff/280001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode212
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:212:
m_idleTaskStatus = IdleTaskCompleted;
Here, add m_elpaseTime += (WTF::monotonicallyIncreasingTime() -
startTime);

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

https://codereview.chromium.org/2039673002/diff/280001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/280001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode199
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:199:
double startTime = WTF::monotonicallyIncreasingTime();

On 2016/06/24 14:56:09, xlai (Olivia) wrote:
> This solution does not look great. Recording the time at every row
encoding adds
> up the duration unnecessarily. I would suggest another approach that
solves this
> issue but won't cause this (pls follow instructions on other
comments).
> First of all, remove this line.

Thanks. Fixed in the new patch.

https://codereview.chromium.org/2039673002/

[xl...@chromium.org]

lgtm. You need approval for the histograms.xml as well.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

+rkaplow: could you take a look at the histogram changes?

https://codereview.chromium.org/2039673002/

[rka...@chromium.org]

https://codereview.chromium.org/2039673002/diff/300001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/300001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode283
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:283:

DEFINE_STATIC_LOCAL(EnumerationHistogram, toBlobIdleTaskStatus,
("Blink.Canvas.ToBlob.IdleTaskStatus", IdleTaskNotSupported));

this should be a MAX value that isn't part of the enum proper (i.e. add
a max value to your enum and use that here)

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

https://codereview.chromium.org/2039673002/diff/300001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
File
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp
(right):

https://codereview.chromium.org/2039673002/diff/300001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp#newcode283
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.cpp:283:
DEFINE_STATIC_LOCAL(EnumerationHistogram, toBlobIdleTaskStatus,
("Blink.Canvas.ToBlob.IdleTaskStatus", IdleTaskNotSupported));

On 2016/06/28 15:14:04, rkaplow wrote:
> this should be a MAX value that isn't part of the enum proper (i.e.
add a max
> value to your enum and use that here)

Done. Also updated in histograms.xml.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

histogram owners: gentle ping.

https://codereview.chromium.org/2039673002/

[ishe...@chromium.org]

https://codereview.chromium.org/2039673002/diff/320001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2039673002/diff/320001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode30
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:30:
// enum used in histogram
nit: Suggested phrasing: "This enum is used to back an UMA histogram,
and should therefore be treated as append-only."

https://codereview.chromium.org/2039673002/diff/320001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode38
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:38:
IdleTaskNotUsed, // Should not be seen in production
nit: I'd name this "IdleTaskCount", which more clearly indicates that it
should remain the last element in the enum.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3584
tools/metrics/histograms/histograms.xml:3584: +
<owner>xl...@chromium.org</owner&gt;
Out of curiousity, why are you not listing yourself as an owner of the
histograms that you're adding?

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3585
tools/metrics/histograms/histograms.xml:3585: + <summary>Time spent on
idle encoding.</summary>
What is "idle encoding"? In general, for events that measure a
duration, it's best to be fairly precise about the start and end events,
since they're not always obvious to someone just viewing the dashboard.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3592
tools/metrics/histograms/histograms.xml:3592: + Records the status of
the idle tasks when finishing a toBlob call.
nit: s/tasks/task ?

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3601
tools/metrics/histograms/histograms.xml:3601: + Time elapsed from
schedule initiate encoding to start initiate encoding.
nit: s/schedule/scheduling and s/start/starting. Also, what does
"initiate encoding" mean, exactly? Would it be semantically valid to
simply drop both uses of "initiate" in this sentence?

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode78494
tools/metrics/histograms/histograms.xml:78494: + <int value="0"
label="IdleTaskNotStarted"/>
nit: These enum labels are fine if you want them to match the C++ code,
but I'd personally recommend more readable names like "Not started",
etc.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode78500
tools/metrics/histograms/histograms.xml:78500: + <int value="6"
label="IdleTaskNotUsed"/>
nit: No need to list the unused overflow bucket in histograms.xml

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode93624
tools/metrics/histograms/histograms.xml:93624: +</histogram_suffixes>
Could the above two histogram_suffixes elements be merged? They share
the same suffixes, and seem likely to continue to do so if more suffixes
are ever added.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

isherman@: all comments are addressed.

https://codereview.chromium.org/2039673002/diff/320001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
File third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h
(right):

https://codereview.chromium.org/2039673002/diff/320001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode30
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:30:
// enum used in histogram

On 2016/06/29 22:01:02, Ilya Sherman wrote:
> nit: Suggested phrasing: "This enum is used to back an UMA histogram,
and should
> therefore be treated as append-only."

Done.

https://codereview.chromium.org/2039673002/diff/320001/third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h#newcode38
third_party/WebKit/Source/core/html/canvas/CanvasAsyncBlobCreator.h:38:
IdleTaskNotUsed, // Should not be seen in production

On 2016/06/29 22:01:02, Ilya Sherman wrote:
> nit: I'd name this "IdleTaskCount", which more clearly indicates that
it should
> remain the last element in the enum.

Done.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3584
tools/metrics/histograms/histograms.xml:3584: +
<owner>xl...@chromium.org</owner&gt;

On 2016/06/29 22:01:02, Ilya Sherman wrote:
> Out of curiousity, why are you not listing yourself as an owner of the
> histograms that you're adding?

xlai@ implements the toBlob function and junov@ is our TL. I believe
that xlai@ will be closely monitoring this histogram in the future.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3585
tools/metrics/histograms/histograms.xml:3585: + <summary>Time spent on
idle encoding.</summary>

On 2016/06/29 22:01:03, Ilya Sherman wrote:
> What is "idle encoding"? In general, for events that measure a
duration, it's
> best to be fairly precise about the start and end events, since
they're not
> always obvious to someone just viewing the dashboard.

Done.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3592
tools/metrics/histograms/histograms.xml:3592: + Records the status of
the idle tasks when finishing a toBlob call.

On 2016/06/29 22:01:03, Ilya Sherman wrote:
> nit: s/tasks/task ?

Done.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode3601
tools/metrics/histograms/histograms.xml:3601: + Time elapsed from
schedule initiate encoding to start initiate encoding.

On 2016/06/29 22:01:03, Ilya Sherman wrote:
> nit: s/schedule/scheduling and s/start/starting. Also, what does
"initiate
> encoding" mean, exactly? Would it be semantically valid to simply
drop both
> uses of "initiate" in this sentence?

Done.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode78500
tools/metrics/histograms/histograms.xml:78500: + <int value="6"
label="IdleTaskNotUsed"/>

On 2016/06/29 22:01:02, Ilya Sherman wrote:
> nit: No need to list the unused overflow bucket in histograms.xml

Done.

https://codereview.chromium.org/2039673002/diff/320001/tools/metrics/histograms/histograms.xml#newcode93624
tools/metrics/histograms/histograms.xml:93624: +</histogram_suffixes>

On 2016/06/29 22:01:03, Ilya Sherman wrote:
> Could the above two histogram_suffixes elements be merged? They share
the same
> suffixes, and seem likely to continue to do so if more suffixes are
ever added.

Done.

https://codereview.chromium.org/2039673002/

[ishe...@chromium.org]

Metrics LGTM % a clarification request, which I think would be great to ask a
teammate to help out with. Thanks =)


https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml#newcode3586
tools/metrics/histograms/histograms.xml:3586: + Time spent on
encoding on the idle period of the main thread.
Sorry, this is still unclear to me. Encoding and idle just don't make
sense to me in combination, since encoding seems like it would, by
definition, not be idle. I'd suggest asking a teammate to help you
clarify this description -- I'd happily offer suggestions, but I'm
really not sure what the intention is.

https://codereview.chromium.org/2039673002/

[xl...@chromium.org]

I put a much lengthy description here on the two metrics that cause confusion.
Not sure whether there is a word limit on metric summary though.

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml#newcode3586
tools/metrics/histograms/histograms.xml:3586: + Time spent on
encoding on the idle period of the main thread.

On 2016/06/30 07:35:51, Ilya Sherman wrote:
> Sorry, this is still unclear to me. Encoding and idle just don't make
sense to
> me in combination, since encoding seems like it would, by definition,
not be
> idle. I'd suggest asking a teammate to help you clarify this
description -- I'd
> happily offer suggestions, but I'm really not sure what the intention
is.

This metric measures the total time spent on encoding all the rows of an
image (jpeg or png), as part of canvas.toBlob API call, which occur
during one or more idle periods on the main thread.

This metric is useful in helping us adjust the
IdleTaskCompleteTimeoutDelay in canvas.toBlob; when the encoding idle
task is delayed for longer than IdleTaskCompleteTimeoutDelay, browser
will switch to non-idle task to force encoding to happen on main thread.

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml#newcode3602
tools/metrics/histograms/histograms.xml:3602: + <summary>Time elapsed
from scheduling encoding to starting encoding.</summary>
This metric measures the time spent from initiating image encoding
(jpeg or png) on idle task to the actual execution time of initiation,
as part of canvas.toBlob API call.

This metric is useful in helping us adjust the IdleTaskStartTimeoutDelay
in
canvas.toBlob; when the initialization idle task is delayed for longer
than
IdleTaskStartTimeoutDelay, browser will switch to non-idle task to force
initialization and encoding to occur on main thread.

https://codereview.chromium.org/2039673002/

[xida...@chromium.org]

https://codereview.chromium.org/2039673002/diff/360001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/360001/tools/metrics/histograms/histograms.xml#newcode3591
tools/metrics/histograms/histograms.xml:3591: +
IdleTaskCompleteTimeoutDelay in canvas.toBlob.
histogram owners: could you please proof read the description here? Both
me and xlai@ are not sure whether this is too long or not.

https://codereview.chromium.org/2039673002/diff/360001/tools/metrics/histograms/histograms.xml#newcode3612
tools/metrics/histograms/histograms.xml:3612: +
IdleTaskStartTimeoutDelay in canvas.toBlob.
histogram owners: please proof read the description here too. Thank you.

https://codereview.chromium.org/2039673002/

[ishe...@chromium.org]

Thanks! The suggested descriptions are much clearer, and LGTM % a few
grammatical nits (inline). There's no length limit on histograms descriptions.

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml
File tools/metrics/histograms/histograms.xml (right):

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml#newcode3586
tools/metrics/histograms/histograms.xml:3586: + Time spent on
encoding on the idle period of the main thread.

On 2016/06/30 14:42:31, xlai (Olivia) wrote:
> On 2016/06/30 07:35:51, Ilya Sherman wrote:
> > Sorry, this is still unclear to me. Encoding and idle just don't
make sense
> to
> > me in combination, since encoding seems like it would, by
definition, not be
> > idle. I'd suggest asking a teammate to help you clarify this
description --
> I'd
> > happily offer suggestions, but I'm really not sure what the
intention is.
>
> This metric measures the total time spent on encoding all the rows of
an image
> (jpeg or png), as part of canvas.toBlob API call, which occur during
one or more
> idle periods on the main thread.
>
> This metric is useful in helping us adjust the
IdleTaskCompleteTimeoutDelay in
> canvas.toBlob; when the encoding idle task is delayed for longer than
> IdleTaskCompleteTimeoutDelay, browser will switch to non-idle task to
force
> encoding to happen on main thread.

nit: s/as part of/as part of a
nit: s/call, which occur/call. Encoding occurs
nit: s/toBlob; when/toBlob. When
nit: s/browser will/the browser will
nit: s/switch to non-idle task/switch to a non-idle task
nit: s/on main thread/on the main thread

https://codereview.chromium.org/2039673002/diff/340001/tools/metrics/histograms/histograms.xml#newcode3602
tools/metrics/histograms/histograms.xml:3602: + <summary>Time elapsed
from scheduling encoding to starting encoding.</summary>

On 2016/06/30 14:42:31, xlai (Olivia) wrote:
> This metric measures the time spent from initiating image encoding
> (jpeg or png) on idle task to the actual execution time of initiation,
as part
> of canvas.toBlob API call.
>
> This metric is useful in helping us adjust the
IdleTaskStartTimeoutDelay in
> canvas.toBlob; when the initialization idle task is delayed for longer
than
> IdleTaskStartTimeoutDelay, browser will switch to non-idle task to
force
> initialization and encoding to occur on main thread.

(similar nits here)

https://codereview.chromium.org/2039673002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ is trying da patch. Follow status at

https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.org/2039673002/400001


https://codereview.chromium.org/2039673002/

[commit-bot@chromium.org via codereview.chromium.org]

Committed patchset #21 (id:400001)

https://codereview.chromium.org/2039673002/

[commit-bot@chromium.org via codereview.chromium.org]

CQ bit was unchecked.

https://codereview.chromium.org/2039673002/

[commit-bot@chromium.org via codereview.chromium.org]

Patchset 21 (id:??) landed as
https://crrev.com/8c59e014125ad442b32c21b1724add603b489cc2
Cr-Commit-Position: refs/heads/master@{#403362}

https://codereview.chromium.org/2039673002/