Build time improvement: [CSS Calc] Collapse RoundNearest/Up/Down/ToZero into a single Round struct#62308
Conversation
|
EWS run on previous version of this PR (hash bc4929f) Details
|
…ro into a single Round struct https://bugs.webkit.org/show_bug.cgi?id=311770 rdar://174359001 Reviewed by NOBODY (OOPS!). Profiling builds has shown that a significant portion of build time is tied up in template instantiation of `mpark::variant` types, primarily because of the very deep nesting involved in WebKit. We can improve the complexity by replacing the four separate round operation types (RoundNearest, RoundUp, RoundDown, RoundToZero) with a single Round struct carrying a RoundingStrategy enum field. This reduces the CSSCalc::Node variant from 37 to 34 alternatives, and the Style::Calculation::Node variant from 32 to 29 alternatives, shrinking mpark::variant template instantiation depth across ~290 translation units. This reduces build time be around 4.3% * Source/WebCore/Headers.cmake: * Source/WebCore/WebCore.xcodeproj/project.pbxproj: * Source/WebCore/css/calc/CSSCalcRoundingStrategy.h: Added. (WebCore::CSSCalc::nameLiteral): * Source/WebCore/css/calc/CSSCalcTree+Copy.cpp: (WebCore::CSSCalc::copy): * Source/WebCore/css/calc/CSSCalcTree+Evaluation.cpp: (WebCore::CSSCalc::evaluate): * Source/WebCore/css/calc/CSSCalcTree+Mappings.h: * Source/WebCore/css/calc/CSSCalcTree+Parser.cpp: (WebCore::CSSCalc::consumeRoundArguments): (WebCore::CSSCalc::consumeRound): * Source/WebCore/css/calc/CSSCalcTree+Serialization.cpp: (WebCore::CSSCalc::serializeMathFunctionPrefix): * Source/WebCore/css/calc/CSSCalcTree+Simplification.cpp: (WebCore::CSSCalc::simplify): (WebCore::CSSCalc::copyAndSimplifyChildren): (WebCore::CSSCalc::simplifyForRound): Deleted. * Source/WebCore/css/calc/CSSCalcTree+Simplification.h: * Source/WebCore/css/calc/CSSCalcTree.cpp: (WebCore::CSSCalc::toType): * Source/WebCore/css/calc/CSSCalcTree.h: (WebCore::CSSCalc::toCSSValueID): (WebCore::CSSCalc::get): * Source/WebCore/style/calc/StyleCalculationTree+Conversion.cpp: (WebCore::Style::Calculation::toCSS): (WebCore::Style::Calculation::toStyle): * Source/WebCore/style/calc/StyleCalculationTree+Copy.cpp: (WebCore::Style::Calculation::copy): * Source/WebCore/style/calc/StyleCalculationTree+Evaluation.cpp: (WebCore::Style::Calculation::evaluate): * Source/WebCore/style/calc/StyleCalculationTree.cpp: (WebCore::Style::Calculation::operator<<): * Source/WebCore/style/calc/StyleCalculationTree.h: (WebCore::Style::Calculation::get):
bc4929f to
3fe5595
Compare
|
EWS run on current version of this PR (hash 3fe5595) Details
|
🛠 ios-apple|
Crazy, but if that is the improvement, wow. Do you know what translation units calc tree types are getting instantiated in? I made an effort to limit this when I originally wrote it, but perhaps that has regressed. |
|
A/B is partially done and shows no regression so far. I won't land until we get a complete run (hopefully by tomorrow). |
I attached some output to the bugzilla just now. It's not super helpful but might be usable to figure out what's going on. I'll see if I can get more complete symbol names for the template instantiations so you can at least see the types being used. The data is primarily about the cost per unified build file, which makes teasing out specific implementation files tedious. Hopefully that's at least some help. |
|
@brentfulgham, out of curiosity, did you try using extern template (like you did for others here #62367) for this? |
weinig
left a comment
There was a problem hiding this comment.
By adding support for the new struct layout in CSSCalcTree+Traversal.h, a lot of the overloads should be reduced to just adding support for the enum value.
| constexpr ASCIILiteral nameLiteral(RoundingStrategy strategy) | ||
| { | ||
| switch (strategy) { | ||
| case RoundingStrategy::Nearest: return ASCIILiteral::fromLiteralUnsafe("nearest"); |
There was a problem hiding this comment.
These should just use the literal syntax "nearest"_s (etc.).
| case RoundingStrategy::Down: return ASCIILiteral::fromLiteralUnsafe("down"); | ||
| case RoundingStrategy::ToZero: return ASCIILiteral::fromLiteralUnsafe("to-zero"); | ||
| } | ||
| return ASCIILiteral::fromLiteralUnsafe("nearest"); |
There was a problem hiding this comment.
You can do a RELEASE_ASSERT_NOT_REACHED() here since the switch is exhaustive.
|
Overall I don't love this, because it doesn't really solve the problem, and it means that with each additional calc node we add (which we will do, there are already some I am working on) it will cause a slowdown. I'd love to find a solution that reduced the number of translation units these got instantiated in, but if that is not possible, I this seems like an ok short term solution. |
3fe5595
3fe5595