Bug 74547 – [Chromium] Add trace events for decoding and drawing images.

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Daniel Sievers]

[Chromium] Add trace events for decoding and drawing images.

[Daniel Sievers]

Created attachment 119308 [details]
Patch

[Nat Duca]

Comment on attachment 119308 [details]
Patch

Neato.

[John Bates]

Comment on attachment 119308 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:81
> +            TRACE_EVENT("NativeImageSkia::resizedBitmap", const_cast<NativeImageSkia*>(this), "noncached");

May want to avoid using the extra string parameter -- it causes a strdup even when tracing is disabled. Hmm or you could remove the strdup from TraceEvent.h and always pass 0 to traceEventEnd. Isn't that redundant info anyway?

[Nat Duca]

(In reply to comment #3)
> May want to avoid using the extra string parameter -- it causes a strdup even when tracing is disabled.

Oh good god, how the @%&()* did that get in there?

Daniel, while you're in this code, can you please make the strdup in TraceEvent.h conditional on PlatformSupport::isTraceEventEnabled, at least?

[Daniel Sievers]

Created attachment 119476 [details]
Patch

[Daniel Sievers]

https://bugs.webkit.org/show_bug.cgi?id=74637 for strdup().

I realized it might add value to have different trace events for the three paths we can hit in resizedBitmap(). That way you can simply select a region and see how often we hit the cache and such.

[John Bates]

Comment on attachment 119476 [details]
Patch

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

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

Better to use {} around this macro -- if I get around to improving it, this code would break.

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

Same here

[Nat Duca]

(In reply to comment #7)
> (From update of attachment 119476 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=119476&action=review
> 
> > Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:86
> > +            TRACE_EVENT("NativeImageSkia::resizedBitmap_cachedResize", const_cast<NativeImageSkia*>(this), 0);
> 
> Better to use {} around this macro -- if I get around to improving it, this code would break.

Woah, no! That's terrible! That'd break the timing measurement its' trying to do?

[John Bates]

Comment on attachment 119476 [details]
Patch

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

>>> Source/WebCore/platform/graphics/skia/NativeImageSkia.cpp:86
>>> +            TRACE_EVENT("NativeImageSkia::resizedBitmap_cachedResize", const_cast<NativeImageSkia*>(this), 0);
>> 
>> Better to use {} around this macro -- if I get around to improving it, this code would break.
> 
> Woah, no! That's terrible! That'd break the timing measurement its' trying to do?

(discussed offline -- it was a misunderstanding)

You may want to change this back to your old impl with the string arg now that you're fixing the strdup. Otherwise these trace events don't catch this whole scope (if that matters).

[Daniel Sievers]

Created attachment 119502 [details]
Patch

[Daniel Sievers]

I think I'd like to keep the separate trace events, just because it's cool to be able to drag a rectangle in the trace viewer and see the occurrence count for both (cached, noncached) separately.

How about this new variant?

> 
> You may want to change this back to your old impl with the string arg now that you're fixing the strdup. Otherwise these trace events don't catch this whole scope (if that matters).

[John Bates]

Comment on attachment 119502 [details]
Patch

lgtm

[James Robinson]

Comment on attachment 119502 [details]
Patch

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

> Source/WebCore/platform/graphics/skia/ImageSkia.cpp:49
> +#include "TraceEvent.h"

skia does not necessarily imply chromium, so you need the same #if PLATFORM() guards here as elsewhere. sorry i didn't catch this the first time

[Daniel Sievers]

Created attachment 119528 [details]
Patch

[James Robinson]

Comment on attachment 119528 [details]
Patch

Awesome.

[WebKit Review Bot]

Comment on attachment 119528 [details]
Patch

Clearing flags on attachment: 119528

Committed r103041: <http://trac.webkit.org/changeset/103041>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.