Bug 89138

Summary: [cairo] improve putByteArray speed by avoiding max/min checks at Color construction.
Product: WebKit Reporter: arno. <a.renevier>
Component: PlatformAssignee: Nobody <webkit-unassigned>
Status: RESOLVED FIXED    
Severity: Normal CC: abarth, igor.oliveira, mrobinson, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
patch proposal
none
updated patch: unsigned char constructor for Color
none
patch v2.1: prevents build failure on mac
none
patch v2.2: using static_cast
none
patch v2.3: Color::createUnChecked function none

Description arno. 2012-06-14 16:57:56 PDT
Hi,
as explained in bug #89130, putByteArray constructs Color objects from unsigned char* elements.
Color constructors takes integers arguments and checks they are between 0 and 255.
As arguments are unsigned char*, we known for sure they're between 0 and 255, so we could probably try to skip this step.
Comment 1 arno. 2012-06-14 17:10:07 PDT
Created attachment 147682 [details]
patch proposal
Comment 2 Martin Robinson 2012-06-18 14:28:35 PDT
Is it possible to simply have an unsigned char constructor for Color?
Comment 3 arno. 2012-06-19 12:03:02 PDT
Created attachment 148385 [details]
updated patch: unsigned char constructor for Color
Comment 4 Martin Robinson 2012-06-19 12:26:58 PDT
Comment on attachment 148385 [details]
updated patch: unsigned char constructor for Color

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

> Source/WebCore/platform/graphics/Color.h:75
> +inline RGBA32 makeRGBUnsafe(int r, int g, int b)
> +{
> +    return 0xFF000000 | r << 16 | g << 8 | b;
> +}
> +
> +inline RGBA32 makeRGBAUnsafe(int r, int g, int b, int a)
> +{
> +    return a << 24 | r << 16 | g << 8 | b;
> +}
> +

Why not simply override these methods as well to take unsigneds?
Comment 5 Build Bot 2012-06-19 13:03:42 PDT
Comment on attachment 148385 [details]
updated patch: unsigned char constructor for Color

Attachment 148385 [details] did not pass mac-ews (mac):
Output: http://queues.webkit.org/results/12992015
Comment 6 arno. 2012-06-19 13:19:07 PDT
Created attachment 148404 [details]
patch v2.1: prevents build failure on mac
Comment 7 Igor Trindade Oliveira 2012-06-19 13:22:08 PDT
Comment on attachment 148404 [details]
patch v2.1: prevents build failure on mac

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

> Source/WebCore/platform/graphics/cg/ImageCG.cpp:131
> +            m_solidColor = Color(pixel[0] * 255 / pixel[3], pixel[1] * 255 / pixel[3], pixel[2] * 255 / pixel[3], (int)pixel[3]);

nit: normally we use c++ cast.
Comment 8 arno. 2012-06-19 15:59:09 PDT
Created attachment 148449 [details]
patch v2.2: using static_cast
Comment 9 arno. 2012-06-19 17:34:47 PDT
(In reply to comment #4)
> (From update of attachment 148385 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=148385&action=review
> 
> > Source/WebCore/platform/graphics/Color.h:75
> > +inline RGBA32 makeRGBUnsafe(int r, int g, int b)
> > +{
> > +    return 0xFF000000 | r << 16 | g << 8 | b;
> > +}
> > +
> > +inline RGBA32 makeRGBAUnsafe(int r, int g, int b, int a)
> > +{
> > +    return a << 24 | r << 16 | g << 8 | b;
> > +}
> > +
> 
> Why not simply override these methods as well to take unsigneds?

Because some parts of the code call makeRGB(float, ), and if we make makeRGB(unsigned char), those calls would become redundant. Also, maybe in some cases, it would be convenient to call makeRGBUnsafe with integers if we already known those integers are between 0 and 255 (I'm thinking for example at the colorFromPremultipliedARGB function)
Comment 10 arno. 2012-06-19 17:57:31 PDT
ccing adam barth as he's among one of the most few people to have review Color.h changes recently.
Comment 11 Adam Barth 2012-06-19 20:07:15 PDT
Is there a measurable performance gain from this change?
Comment 12 arno. 2012-06-20 11:15:03 PDT
(In reply to comment #11)
> Is there a measurable performance gain from this change?

http://renevier.net/misc/webkit_89138.html
calls getImageData 100 times. Time it takes goes from 220ms to 165ms with GtkLauncher (185ms if function is not inlined)
Comment 13 Adam Barth 2012-06-20 12:58:24 PDT
This change seems valuable, but I'm slightly worried that it introduces a subtle overloading of the constructor.  I wonder if it would make more sense to introduce a new static function for creating these objects so callers can be explicit about whether they want to unchecked version.
Comment 14 arno. 2012-06-20 14:35:55 PDT
Created attachment 148657 [details]
patch v2.3: Color::createUnChecked function
Comment 15 Adam Barth 2012-06-20 17:59:08 PDT
Comment on attachment 148657 [details]
patch v2.3: Color::createUnChecked function

This looks much better.  Thanks.
Comment 16 WebKit Review Bot 2012-06-20 23:09:04 PDT
Comment on attachment 148657 [details]
patch v2.3: Color::createUnChecked function

Clearing flags on attachment: 148657

Committed r120905: <http://trac.webkit.org/changeset/120905>
Comment 17 WebKit Review Bot 2012-06-20 23:09:09 PDT
All reviewed patches have been landed.  Closing bug.