Summary: | Speed up compilation of tests involving WebTransformationMatrix | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | nbhargava | ||||||||||
Component: | New Bugs | Assignee: | 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
nbhargava
2012-08-21 15:53:40 PDT
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
I'm OK with this as long as James or other compositor folks are. Theyr'e the ones who use these tests I think. This makes the test failures messages pretty much completely useless. How far out would a clang fix be? 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 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? Yes, that's what I'm suggesting. Created attachment 159815 [details]
Patch
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
Comment on attachment 159995 [details]
Patch
Yay!
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 on attachment 159995 [details] Patch Clearing flags on attachment: 159995 Committed r126352: <http://trac.webkit.org/changeset/126352> All reviewed patches have been landed. Closing bug. 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 |