Bug 94639

Summary: Speed up compilation of tests involving WebTransformationMatrix
Product: WebKit Reporter: nbhargava
Component: New BugsAssignee: nbhargava
Status: RESOLVED FIXED    
Severity: Normal CC: eric, jamesr, shawnsingh, webkit.review.bot
Priority: P2    
Version: 528+ (Nightly build)   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch
none
Patch
none
Patch none

Description nbhargava 2012-08-21 15:53:40 PDT
Speed up compilation of tests involving WebTransformationMatrix
Comment 1 nbhargava 2012-08-21 15:57:55 PDT
Created attachment 159786 [details]
Patch
Comment 2 nbhargava 2012-08-21 16:01:16 PDT
EXPECT_TRANSFORMATION_MATRIX_EQ was changed from a macro to a function. As a macro that expanded into 16 other macros, it caused an unneeded slow down of close to 45s in the compile time of CCLayerTreeHostCommonTest.cpp.

The one thing that is lost in this change is a more precise reporting of line numbers when the tests fail. However, given that tests don't fail often, but people may recompile files fairly often, it makes more sense to cut down on a somewhat large overhead.

Bugs have been filed to clang and gcc noting the errant compiler behavior: http://llvm.org/bugs/show_bug.cgi?id=13651 and http://gcc.gnu.org/bugzilla/show_bug.cgi?id=54337
Comment 3 nbhargava 2012-08-21 16:15:28 PDT
Created attachment 159790 [details]
Patch
Comment 4 Eric Seidel (no email) 2012-08-21 16:43:55 PDT
I'm OK with this as long as James or other compositor folks are.  Theyr'e the ones who use these tests I think.
Comment 5 James Robinson 2012-08-21 16:46:02 PDT
This makes the test failures messages pretty much completely useless.  How far out would a clang fix be?
Comment 6 James Robinson 2012-08-21 16:46:34 PDT
Alternately, could you play around with the gtest failure macros to try to make the failure message still useful but still use a function?  It has some functionality for reporting errors at specific places.
Comment 7 James Robinson 2012-08-21 16:49:03 PDT
Comment on attachment 159790 [details]
Patch

Perhaps EXPECT_PRED2() could solve this, or perhaps SCOPED_TRACE()?

r- until you can make failures point to the real location. it doesn't appear to be too hard.
Comment 8 nbhargava 2012-08-21 16:49:20 PDT
I'm not sure how far out a clang fix is.

Given that it makes line number stuff difficult, I was looking at this http://code.google.com/p/googletest/wiki/AdvancedGuide#Using_Assertions_in_Sub-routines. Is it worth potentially adding this?
Comment 9 James Robinson 2012-08-21 16:57:44 PDT
Yes, that's what I'm suggesting.
Comment 10 nbhargava 2012-08-21 17:39:01 PDT
Created attachment 159815 [details]
Patch
Comment 11 James Robinson 2012-08-21 17:45:11 PDT
Comment on attachment 159815 [details]
Patch

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

> Source/WebKit/chromium/tests/CCLayerTreeHostCommonTest.cpp:188
> +    {
> +        SCOPED_TRACE("");
> +        ExpectTransformationMatrixEq(identityMatrix, child->drawTransform());
> +    }

instead of having to change all of these lines, could you have EXPECT_TRANS...() expand to a macro that creates a new scope, SCOPED_TRACE(), and calls the matcher function?
Comment 12 nbhargava 2012-08-22 13:07:12 PDT
Created attachment 159995 [details]
Patch
Comment 13 James Robinson 2012-08-22 13:08:13 PDT
Comment on attachment 159995 [details]
Patch

Yay!
Comment 14 James Robinson 2012-08-22 13:08:44 PDT
I'm not sure if you are a webkit committer or not - if not, you can set the "commit-queue" flag to ? and a committer will put it into the commit queue for you.
Comment 15 WebKit Review Bot 2012-08-22 14:28:41 PDT
Comment on attachment 159995 [details]
Patch

Clearing flags on attachment: 159995

Committed r126352: <http://trac.webkit.org/changeset/126352>
Comment 16 WebKit Review Bot 2012-08-22 14:28:45 PDT
All reviewed patches have been landed.  Closing bug.
Comment 17 Shawn Singh 2012-08-22 21:50:41 PDT
Just a post-landing follow up.  I just now saw this code.

I think we should still encourage the use of EXPECT_TRANSFORMATION_MATRIX_EQ macro, so that this can be changed later if we want?

If you guys agree, I'll add a comment on this function as a drive by to one of my upcoming patches - https://bugs.webkit.org/show_bug.cgi?id=94542