Added DecodeImage and ResizeImage events to TimelineAgent
Created attachment 150164 [details] Patch
Comment on attachment 150164 [details] Patch Attachment 150164 [details] did not pass win-ews (win): Output: http://queues.webkit.org/results/13121301
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?
Comment on attachment 150164 [details] Patch Attachment 150164 [details] did not pass qt-wk2-ews (qt): Output: http://queues.webkit.org/results/13119331
Comment on attachment 150164 [details] Patch Attachment 150164 [details] did not pass chromium-ews (chromium-xvfb): Output: http://queues.webkit.org/results/13111379
Comment on attachment 150164 [details] Patch Attachment 150164 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13119323
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.
Created attachment 150173 [details] Patch
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.
Created attachment 150186 [details] Patch
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
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
Comment on attachment 150186 [details] Patch I don't think you've addressed my comments.
Created attachment 150379 [details] Patch
Comment on attachment 150379 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=150379&action=review > Source/WebCore/ChangeLog:46 > +2012-06-29 Sergey Rogulenko <rogulenko@google.com> 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.
Created attachment 150914 [details] Patch
Created attachment 150916 [details] Patch
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.
Created attachment 151043 [details] Patch
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.
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.
Created attachment 151050 [details] Patch
Comment on attachment 151050 [details] Patch Attachment 151050 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13145413
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.
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.
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
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
Created attachment 151694 [details] Patch
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?
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
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
Created attachment 151900 [details] Patch
Created attachment 151904 [details] Patch
Comment on attachment 151904 [details] Patch LGTM. Pavel?
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?
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?
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).
Created attachment 155516 [details] Patch
Comment on attachment 155516 [details] Patch This change should be tested.
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.
Created attachment 155794 [details] Patch
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?
Created attachment 156364 [details] Patch
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.
Created attachment 156386 [details] Patch
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.
(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?
(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.
> 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.
(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.
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.
(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.
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
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
Comment on attachment 156386 [details] Patch Clearing r? for now.
Extracted low-level platform instrumentation as a separate patch in bug 94125.
Created attachment 158784 [details] Patch
(In reply to comment #57) > Created an attachment (id=158784) [details] > Patch Rebased Sergey's patch on top of bug 94125.
Comment on attachment 158784 [details] Patch I think this change was originally authored by Sergey, why did the ChangeLog change?
Created attachment 158839 [details] Patch
Comment on attachment 158839 [details] Patch Clearing flags on attachment: 158839 Committed r125790: <http://trac.webkit.org/changeset/125790>
All reviewed patches have been landed. Closing bug.