Web Inspector: added Paint events for Images to TimelineAgent

• Component: Web Inspector (Deprecated)
• Status: RESOLVED FIXED

[Sergey Rogulenko]

Added DecodeImage and ResizeImage events to TimelineAgent

[Sergey Rogulenko]

Created attachment 150164 [details]
Patch

[Build Bot]

Comment on attachment 150164 [details]
Patch

Attachment 150164 [details] did not pass win-ews (win):
Output: http://queues.webkit.org/results/13121301

[Andrey Kosyakov]

Comment on attachment 150164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150164&action=review

> Source/WebCore/inspector/InspectorInstrumentation.h:846
> +    if (InspectorTimelineAgent::instance())
> +        InspectorTimelineAgent::instance()->willDecodeImage(rect);

Can we make these instrumentation methods static? This will also let us keep instance() private.

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:447
> +    InspectorInstrumentation::willDecodeImage(IntRect(dstRect));

How did you pick this particular instrumentation point? What about other platforms?

[Early Warning System Bot]

Comment on attachment 150164 [details]
Patch

Attachment 150164 [details] did not pass qt-wk2-ews (qt):
Output: http://queues.webkit.org/results/13119331

[WebKit Review Bot]

Comment on attachment 150164 [details]
Patch

Attachment 150164 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13111379

[Build Bot]

Comment on attachment 150164 [details]
Patch

Attachment 150164 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13119323

[Pavel Feldman]

Comment on attachment 150164 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150164&action=review

> Source/WebCore/ChangeLog:8
> +        * inspector/InspectorInstrumentation.h:

Please explain what happens and why above this line.

> Source/WebCore/inspector/InspectorInstrumentation.h:843
> +    if (!hasFrontends())

Please use FAST_RETURN_IF_NO_FRONTENDS macro

> Source/WebCore/inspector/InspectorInstrumentation.h:845
> +    if (InspectorTimelineAgent::instance())

You can not include InspectorTimelineAgent.h in instrumentation, so this would not work.

>> Source/WebCore/inspector/InspectorInstrumentation.h:846
>> +        InspectorTimelineAgent::instance()->willDecodeImage(rect);
> 
> Can we make these instrumentation methods static? This will also let us keep instance() private.

You need to call willDecodeImagImpl on the instrumentation instance here.

> Source/WebCore/inspector/TimelineRecordFactory.cpp:184
> +    data->setNumber("x", rect.x());

Please rename to createImageData and use in both cases.

> Source/WebCore/inspector/front-end/TimelineModel.js:53
> +    DecodeImage: "DecodeImage",

This will make the tests fail, you need to update expectations.

> Source/WebCore/inspector/front-end/TimelinePanel.js:133
> +    this._presentationModel.addFilter(new WebInspector.TimelineCategoryFilter());

You should not mix style changes with semantics changes.

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:76
> +    recordStyles[recordTypes.DecodeImage] = { title: WebInspector.UIString("Decode Image"), category: categories["painting"] };

New strings should go to the English.lproj/localizedStrings.js as well.

[Sergey Rogulenko]

Created attachment 150173 [details]
Patch

[Pavel Feldman]

Comment on attachment 150173 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150173&action=review

As per my earlier comments (good that some of them are already addressed)

> Source/WebCore/inspector/InspectorInstrumentation.cpp:477
> +    InspectorTimelineAgent::willDecodeImage(rect);

I'd use s_timelineAgent here and move setTimelineAgent into the instrumentation.

[Sergey Rogulenko]

Created attachment 150186 [details]
Patch

[WebKit Review Bot]

Comment on attachment 150186 [details]
Patch

Attachment 150186 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13107513

New failing tests:
inspector/timeline/timeline-enum-stability.html

[WebKit Review Bot]

Created attachment 150236 [details]
Archive of layout-test-results from ec2-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: ec2-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.35-28-virtual-x86_64-with-Ubuntu-10.10-maverick

[Pavel Feldman]

Comment on attachment 150186 [details]
Patch

I don't think you've addressed my comments.

[Sergey Rogulenko]

Created attachment 150379 [details]
Patch

[Pavel Feldman]

Comment on attachment 150379 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150379&action=review

> Source/WebCore/ChangeLog:46
> [phone]  Sergey Rogulenko  <[e-mail]>

You should only include one change log entry.

> Source/WebCore/inspector/InspectorTimelineAgent.h:103
> +    static void willDecodeImage(const LayoutRect&);

Timeline should not expose public instrumentation methods to outside WebCore/inspector.

[Sergey Rogulenko]

Created attachment 150914 [details]
Patch

[Sergey Rogulenko]

Created attachment 150916 [details]
Patch

[Pavel Feldman]

Comment on attachment 150916 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=150916&action=review

> Source/WebCore/ChangeLog:8
> +        * inspector/InspectorInstrumentation.cpp:

Please explain what has changed and why.

> Source/WebCore/inspector/InspectorInstrumentation.h:261
> +    static InspectorTimelineAgent* timelineAgentForOrphanEvents();

I thought this has already landed. Do you need to rebaseline?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:43
> +#include "InspectorInstrumentation.h"

ditto

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:250
> +    pushCurrentRecord(TimelineRecordFactory::createPaintData(rect), TimelineRecordType::Paint, true, frame, true);

ditto

> Source/WebCore/platform/graphics/GraphicsContext.cpp:494
> +    InspectorInstrumentation::willPaintImage(FractionalLayoutRect(dest.location(), FloatSize(tw, th)));

You should do no extra work in case there are no front-ends. I.e. you should pass dest, tw and th into the method.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:87
>              TRACE_EVENT("nonCachedResize", const_cast<NativeImageSkia*>(this), 0);

I would either move this TRACE_EVENT into the instrumentation or make your instrumentation be called from the macro.

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:89
> +            InspectorInstrumentation::willResizeImage();         

See how TRACE_EVENT above differentiates between the cached and non-cached resizes. You might want to do the same.

[Sergey Rogulenko]

Created attachment 151043 [details]
Patch

[Yury Semikhatsky]

Comment on attachment 151043 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151043&action=review

> Source/WebCore/inspector/InspectorInstrumentation.h:147
> +    static void willPaintImage(const FloatRect&);

You should add forward declaration for FloatRect.

[Andrey Kosyakov]

Comment on attachment 151043 [details]
Patch

Do you think we need paint event for all images? Are these expensive? How would it look for scrolling a page with large number of images? I thought we were aiming at instrumenting operations such as decode and resize, that are less frequent and are often slow.

[Sergey Rogulenko]

Created attachment 151050 [details]
Patch

[Build Bot]

Comment on attachment 151050 [details]
Patch

Attachment 151050 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13145413

[Andrey Kosyakov]

Comment on attachment 151050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151050&action=review

> Source/WebCore/inspector/InspectorInstrumentation.h:158
> +    static void willInspectOrphanEvent(const char*);
> +    static void didInspectOrphanEvent(const char*);

Why would we limit this interface to orphan events only? Inspect also seems an unfortunate choice of word. reportTimelineEventStart()/reportTimelineEventFinish() may be?

> Source/WebCore/inspector/InspectorTimelineAgent.cpp:69
> +static const char OrphanEvent[] = "OrphanEvent";

I don't think we have to share same record type for all generic events.

[Andrey Kosyakov]

Comment on attachment 151050 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151050&action=review

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:-87
> -#if PLATFORM(CHROMIUM)
> -            TRACE_EVENT("nonCachedResize", const_cast<NativeImageSkia*>(this), 0);
> -#endif

Please do not remove tracing instrumentation before we can offer a replacement.

[WebKit Review Bot]

Comment on attachment 151050 [details]
Patch

Attachment 151050 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13156117

New failing tests:
inspector/timeline/timeline-enum-stability.html

[WebKit Review Bot]

Created attachment 151069 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Sergey Rogulenko]

Created attachment 151694 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 151694 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151694&action=review

> Source/WebCore/English.lproj/localizedStrings.js:297
> +localizedStrings["Image Decode"] = "Image Decode";
> +localizedStrings["Image Resize"] = "Image Resize";

Image Decode => Decode Image
Image Resize => Resize Image

> Source/WebCore/inspector/TimelineRecordFactory.cpp:185
> +PassRefPtr<InspectorObject> TimelineRecordFactory::createResizeImageData(bool shouldCache)
> +{
> +    RefPtr<InspectorObject> data = InspectorObject::create();
> +    data->setBoolean("shouldCache", shouldCache);

shouldCache => cached?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:578
> +            case recordTypes.DecodeImage:
> +                break;
> +            case recordTypes.ResizeImage:
> +                break;

Any reasons why we don't want default branch to run?

> Source/WebCore/inspector/front-end/TimelinePresentationModel.js:628
> +                return this.data["shouldCache"] ? "cached" : "non-cached";

use WebInspector.UIString()?

> Source/WebCore/platform/graphics/ImageSource.cpp:150
> +    InspectorInstrumentation::willDecodeImage();
>  
>      ImageFrame* buffer = m_decoder->frameBufferAtIndex(index);
> +    
> +    InspectorInstrumentation::didDecodeImage();

So will this run only when we actually decode image?

[WebKit Review Bot]

Comment on attachment 151694 [details]
Patch

Attachment 151694 [details] did not pass chromium-ews (chromium-xvfb):
Output: http://queues.webkit.org/results/13199306

New failing tests:
inspector/timeline/timeline-enum-stability.html

[WebKit Review Bot]

Created attachment 151709 [details]
Archive of layout-test-results from gce-cr-linux-01

The attached test failures were seen while running run-webkit-tests on the chromium-ews.
Bot: gce-cr-linux-01  Port: <class 'webkitpy.common.config.ports.ChromiumXVFBPort'>  Platform: Linux-2.6.39-gcg-201203291735-x86_64-with-Ubuntu-10.04-lucid

[Sergey Rogulenko]

Created attachment 151900 [details]
Patch

[Sergey Rogulenko]

Created attachment 151904 [details]
Patch

[Andrey Kosyakov]

Comment on attachment 151904 [details]
Patch

LGTM. Pavel?

[Pavel Feldman]

Comment on attachment 151904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151904&action=review

> Source/WebCore/ChangeLog:8
> +        * English.lproj/localizedStrings.js:

Please explain what has changed and why.

> Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp:86
> +        InspectorInstrumentation::willDecodeImage("BMP");

I can see existing TRACE_EVENT called from within decode. Is there a reason we want to put our instrumentation elsewhere? Also, could use introduce RAII concept (InspectorInstrumentation::TraceEvent("ImageDecode") trace;) for this kind of instrumentation?

[Andrey Kosyakov]

Comment on attachment 151904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151904&action=review

>> Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp:86
>> +        InspectorInstrumentation::willDecodeImage("BMP");
> 
> I can see existing TRACE_EVENT called from within decode. Is there a reason we want to put our instrumentation elsewhere? Also, could use introduce RAII concept (InspectorInstrumentation::TraceEvent("ImageDecode") trace;) for this kind of instrumentation?

We'd like to only show full image decodes, as opposed to size only, as the latter are presumed to be cheap -- hence the instrumentation is at the (only) call site that requests full decode. This may be moved instead to the location of TRACE, but will have an extra conditional -- do you prefer this?

[Pavel Feldman]

Comment on attachment 151904 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=151904&action=review

>>> Source/WebCore/platform/image-decoders/bmp/BMPImageDecoder.cpp:86
>>> +        InspectorInstrumentation::willDecodeImage("BMP");
>> 
>> I can see existing TRACE_EVENT called from within decode. Is there a reason we want to put our instrumentation elsewhere? Also, could use introduce RAII concept (InspectorInstrumentation::TraceEvent("ImageDecode") trace;) for this kind of instrumentation?
> 
> We'd like to only show full image decodes, as opposed to size only, as the latter are presumed to be cheap -- hence the instrumentation is at the (only) call site that requests full decode. This may be moved instead to the location of TRACE, but will have an extra conditional -- do you prefer this?

I think you should cut it on the agent level (do not send to the front-end pure header decodes).

[Sergey Rogulenko]

Created attachment 155516 [details]
Patch

[Pavel Feldman]

Comment on attachment 155516 [details]
Patch

This change should be tested.

[Stephen White]

Comment on attachment 155516 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155516&action=review

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:37
> +#include "InspectorInstrumentation.h"

I think this is a layering violation.  In general, files in platform/ shouldn't refer to anything outside of platform/.  You'll probably have to set up some kind of callback.

[Sergey Rogulenko]

Created attachment 155794 [details]
Patch

[Pavel Feldman]

Comment on attachment 155794 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=155794&action=review

> LayoutTests/inspector/timeline/timeline-decode-resize-expected.txt:9
> +        imageType : "PNG"

Please test every image type.

> LayoutTests/inspector/timeline/timeline-decode-resize-expected.txt:32
> +ResizeImage Properties:

Please cache cached: true and false cases.

I suspect this test is flaky: is there a guarantee you are getting exactly 2 resize events on all platforms?

[Sergey Rogulenko]

Created attachment 156364 [details]
Patch

[Pavel Feldman]

Comment on attachment 156364 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=156364&action=review

> LayoutTests/inspector/timeline/timeline-decode-resize.html:61
> +        if (recordCount == 14)

This not the right way of landing it.

[Sergey Rogulenko]

Created attachment 156386 [details]
Patch

[Stephen White]

I'd like to re-iterate:  this is a layering violation.  Nothing in platform/ should depend on files outside of platform/.  (Yes, there are exceptions, but we should be removing those, rather than adding new ones).

In particular, I see that inspector/ depends on dom/ and friends, which means this change introduces a new cyclic dependency between platform/ and dom/.

See https://lists.webkit.org/pipermail/webkit-dev/2012-January/019043.html.

[Pavel Feldman]

(In reply to comment #46)
> I'd like to re-iterate:  this is a layering violation.  Nothing in platform/ should depend on files outside of platform/.  (Yes, there are exceptions, but we should be removing those, rather than adding new ones).
> 
> In particular, I see that inspector/ depends on dom/ and friends, which means this change introduces a new cyclic dependency between platform/ and dom/.
> 
> See https://lists.webkit.org/pipermail/webkit-dev/2012-January/019043.html.

Fixing it would require addition indirection or piggy-backing on the TRACE_EVENT. Is the latter kosher?

[Stephen White]

(In reply to comment #47)
> (In reply to comment #46)
> > I'd like to re-iterate:  this is a layering violation.  Nothing in platform/ should depend on files outside of platform/.  (Yes, there are exceptions, but we should be removing those, rather than adding new ones).
> > 
> > In particular, I see that inspector/ depends on dom/ and friends, which means this change introduces a new cyclic dependency between platform/ and dom/.
> > 
> > See https://lists.webkit.org/pipermail/webkit-dev/2012-January/019043.html.
> 
> Fixing it would require addition indirection or piggy-backing on the TRACE_EVENT. Is the latter kosher?

You'd have to ask Nat Duca about that.

[Pavel Feldman]

> You'd have to ask Nat Duca about that.

I'll talk to Nat, that's not what I am asking. I am double-checking if TRACE_EVENT is kosher from the layering perspective.

[Stephen White]

(In reply to comment #49)
> > You'd have to ask Nat Duca about that.
> 
> I'll talk to Nat, that's not what I am asking. I am double-checking if TRACE_EVENT is kosher from the layering perspective.

I've wondered that myself.  However, it appears to be in chromium's base/, which is very low-level, but most importantly does not depend on WebKit, so it can't introduce any cyclic dependencies.  So I think it's ok.

I don't know a lot about the inspector, but there might be some additional benefits to syncing up events between the inspector and tracing; I think this is something people have mentioned in the past, if only in passing.

[James Robinson]

In WebKit, TRACE_EVENT is provided by Source/WebCore/platform/chromium/TraceEvent.h and is thus fine to use (from a layering perspective) from chromium-specific code anywhere in WebCore.

[James Robinson]

(In reply to comment #51)
> In WebKit, TRACE_EVENT is provided by Source/WebCore/platform/chromium/TraceEvent.h and is thus fine to use (from a layering perspective) from chromium-specific code anywhere in WebCore.

That said, the fact that it's in WebCore/platform (and used from WebCore/platform) means that it cannot depend on inspector/ or anything else in WebCore that's not itself in WebCore/platform.

[Build Bot]

Comment on attachment 156386 [details]
Patch

Attachment 156386 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/13436077

New failing tests:
inspector/timeline/timeline-decode-resize.html

[Build Bot]

Created attachment 156483 [details]
Archive of layout-test-results from apple-mac-3

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: apple-mac-3  Port: <class 'webkitpy.common.config.ports.MacPort'>  Platform: Mac OS X 10.7.4

[Pavel Feldman]

Comment on attachment 156386 [details]
Patch

Clearing r? for now.

[Andrey Kosyakov]

Extracted low-level platform instrumentation as a separate patch in bug 94125.

[Andrey Kosyakov]

Created attachment 158784 [details]
Patch

[Andrey Kosyakov]

(In reply to comment #57)
> Created an attachment (id=158784) [details]
> Patch

Rebased Sergey's patch on top of bug 94125.

[Pavel Feldman]

Comment on attachment 158784 [details]
Patch

I think this change was originally authored by Sergey, why did the ChangeLog change?

[Sergey Rogulenko]

Created attachment 158839 [details]
Patch

[WebKit Review Bot]

Comment on attachment 158839 [details]
Patch

Clearing flags on attachment: 158839

Committed r125790: <http://trac.webkit.org/changeset/125790>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.