Bug 94639 – Speed up compilation of tests involving WebTransformationMatrix

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

Speed up compilation of tests involving WebTransformationMatrix

Created attachment 159786 [details]
Patch

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

Created attachment 159790 [details]
Patch

[Eric Seidel (no email)]

I'm OK with this as long as James or other compositor folks are.  Theyr'e the ones who use these tests I think.

[James Robinson]

This makes the test failures messages pretty much completely useless.  How far out would a clang fix be?

[James Robinson]

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.

[James Robinson]

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.

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?

[James Robinson]

Yes, that's what I'm suggesting.

Created attachment 159815 [details]
Patch

[James Robinson]

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?

Created attachment 159995 [details]
Patch

[James Robinson]

Comment on attachment 159995 [details]
Patch

Yay!

[James Robinson]

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.

[WebKit Review Bot]

Comment on attachment 159995 [details]
Patch

Clearing flags on attachment: 159995

Committed r126352: <http://trac.webkit.org/changeset/126352>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Shawn Singh]

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