calc() values resulting from blending mixed type lengths should be simplified
Created attachment 425494 [details] Patch
Created attachment 425504 [details] Patch
Created attachment 425505 [details] Patch
Comment on attachment 425505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review > Source/WebCore/ChangeLog:12 > + When seraliazing calc() values, we now check whether they are the result of blending seraliazing > Source/WebCore/css/CSSCalculationValue.cpp:2054 > + // CSSCalcOperationNode is only ever created in the context of blending > + // and requires simplification to match serialization rules. That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes
(In reply to Simon Fraser (smfr) from comment #4) > Comment on attachment 425505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425505&action=review > > > Source/WebCore/css/CSSCalculationValue.cpp:2054 > > + // CSSCalcOperationNode is only ever created in the context of blending > > + // and requires simplification to match serialization rules. > > That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, > CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification?
Seems like we only need to simplify after blending.
Comment on attachment 425505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review >>> Source/WebCore/css/CSSCalculationValue.cpp:2054 >>> + // and requires simplification to match serialization rules. >> >> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes > > Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification? It seems really weird that blending would require different rules than other places. Where is this specified?
(In reply to Sam Weinig from comment #7) > Comment on attachment 425505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425505&action=review > > >>> Source/WebCore/css/CSSCalculationValue.cpp:2054 > >>> + // and requires simplification to match serialization rules. > >> > >> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes > > > > Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification? > > It seems really weird that blending would require different rules than other > places. Where is this specified? It doesn't require different rules. It's just that we normally simplify after parsing, and blending generates calc() values which are not the result of parsing.
Created attachment 425525 [details] Patch
Comment on attachment 425505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review > Source/WebCore/css/CSSCalculationValue.cpp:2056 > + if (is<CSSCalcOperationNode>(node)) > + node = CSSCalcOperationNode::simplify(node.get()); This is what virtual functions are for. Is there a good reason we don’t have a virtual function version of simplify?
Comment on attachment 425505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review >> Source/WebCore/css/CSSCalculationValue.cpp:2056 >> + node = CSSCalcOperationNode::simplify(node.get()); > > This is what virtual functions are for. Is there a good reason we don’t have a virtual function version of simplify? Presumably it would just return "this" on non-calc nodes.
Comment on attachment 425505 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425505&action=review >>>>> Source/WebCore/css/CSSCalculationValue.cpp:2054 >>>>> + // and requires simplification to match serialization rules. >>>> >>>> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes >>> >>> Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification? >> >> It seems really weird that blending would require different rules than other places. Where is this specified? > > It doesn't require different rules. It's just that we normally simplify after parsing, and blending generates calc() values which are not the result of parsing. Could we use a dirty bit to say "already simplified" to deal with this?
(In reply to Darin Adler from comment #12) > Comment on attachment 425505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425505&action=review > > >>>>> Source/WebCore/css/CSSCalculationValue.cpp:2054 > >>>>> + // and requires simplification to match serialization rules. > >>>> > >>>> That doesn't seem accurate. CSSCalcOperationNode::createMinOrMaxOrClamp, CSSCalcOperationNode::createProduct all return CSSCalcOperationNodes > >>> > >>> Oh! Should we keep a flag of when we create one for blending and only simplify then, or do those cases also need simplification? > >> > >> It seems really weird that blending would require different rules than other places. Where is this specified? > > > > It doesn't require different rules. It's just that we normally simplify after parsing, and blending generates calc() values which are not the result of parsing. > > Could we use a dirty bit to say "already simplified" to deal with this? We could, it felt to me like more work that necessary to manage setting that bit to false. We generally know that these values need simplification as being, most likely, the result of blending, and this codepath is only used for serializing computed values, so it's unlikely we'll call the simplification code more than necessary.
(In reply to Darin Adler from comment #11) > Comment on attachment 425505 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=425505&action=review > > >> Source/WebCore/css/CSSCalculationValue.cpp:2056 > >> + node = CSSCalcOperationNode::simplify(node.get()); > > > > This is what virtual functions are for. Is there a good reason we don’t have a virtual function version of simplify? > > Presumably it would just return "this" on non-calc nodes. I don't follow here. This is not about requiring special work to be done for CSSCalcOperationNode, it's just that in the other cases we know simplification happened already at parse-time, whereas for these we don't simplify when creating them – and don't need to until we serialize them.
Can you point me to the blending code that is calling this? This really feels like the wrong place to be doing this.
(In reply to Sam Weinig from comment #15) > Can you point me to the blending code that is calling this? This really > feels like the wrong place to be doing this. https://github.com/WebKit/WebKit/blob/0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length.cpp#L297
(In reply to Antoine Quint from comment #16) > (In reply to Sam Weinig from comment #15) > > Can you point me to the blending code that is calling this? This really > > feels like the wrong place to be doing this. > > https://github.com/WebKit/WebKit/blob/ > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length. > cpp#L297 Hm, can you walk me through it a bit more. That creates a CalcExpressionBlendLength. Where does the CSSCalcValue come from?
This is the first time I look at the calc() code, so it’s quite likely there’s a better way to do this. However, it does seem like simplification is directly related to serialization, and this looked like a good place to do this. Another option is to simplify as we create the expression object, but we don’t need to simplify aside from serialization, so it sounds wasteful.
(In reply to Sam Weinig from comment #17) > (In reply to Antoine Quint from comment #16) > > (In reply to Sam Weinig from comment #15) > > > Can you point me to the blending code that is calling this? This really > > > feels like the wrong place to be doing this. > > > > https://github.com/WebKit/WebKit/blob/ > > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length. > > cpp#L297 > > Hm, can you walk me through it a bit more. That creates a > CalcExpressionBlendLength. > > Where does the CSSCalcValue come from? We really need Simon to help here, I think he wrote most of this code.
(In reply to Sam Weinig from comment #17) > (In reply to Antoine Quint from comment #16) > > (In reply to Sam Weinig from comment #15) > > > Can you point me to the blending code that is calling this? This really > > > feels like the wrong place to be doing this. > > > > https://github.com/WebKit/WebKit/blob/ > > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length. > > cpp#L297 > > Hm, can you walk me through it a bit more. That creates a > CalcExpressionBlendLength. > > Where does the CSSCalcValue come from? I think I worked it out. I think it probably the CSSPrimitiveValue constructor that takes a Length and when it is a calculated length calls `CSSCalcValue::create(length.calculationValue(), style)`. I think the right time to do simplification is in CSSCalcValue::create(const CalculationValue&, const RenderStyle&). Just as the CSSCalcValue::create(CSSValueID, const CSSParserTokenRange&, CalculationCategory) does simplification via parser.parseCalc(), the create taking a CalculationValue should do it upfront. This will maintain the invariant that CSSCalcValue is always in its simplified form. If we want to optimize not simplifying until it is actually needed, we could do that for both forms, but I don't think that is required.
(In reply to Sam Weinig from comment #20) > (In reply to Sam Weinig from comment #17) > > (In reply to Antoine Quint from comment #16) > > > (In reply to Sam Weinig from comment #15) > > > > Can you point me to the blending code that is calling this? This really > > > > feels like the wrong place to be doing this. > > > > > > https://github.com/WebKit/WebKit/blob/ > > > 0ebbd8f1ce40d6a78dbe72a826a20a1994c9d0dd/Source/WebCore/platform/Length. > > > cpp#L297 > > > > Hm, can you walk me through it a bit more. That creates a > > CalcExpressionBlendLength. > > > > Where does the CSSCalcValue come from? > > I think I worked it out. I think it probably the CSSPrimitiveValue > constructor that takes a Length and when it is a calculated length calls > `CSSCalcValue::create(length.calculationValue(), style)`. > > I think the right time to do simplification is in CSSCalcValue::create(const > CalculationValue&, const RenderStyle&). Just as the > CSSCalcValue::create(CSSValueID, const CSSParserTokenRange&, > CalculationCategory) does simplification via parser.parseCalc(), the create > taking a CalculationValue should do it upfront. This will maintain the > invariant that CSSCalcValue is always in its simplified form. > > If we want to optimize not simplifying until it is actually needed, we could > do that for both forms, but I don't think that is required. Something like this: RefPtr<CSSCalcValue> CSSCalcValue::create(const CalculationValue& value, const RenderStyle& style) { auto expression = createCSS(value.expression(), style); if (!expression) return nullptr; auto simplifiedExpression = CSSCalcOperationNode::simplify(expression.releaseNonNull()); auto result = adoptRef(new CSSCalcValue(WTFMove(simplifiedExpression), value.shouldClampToNonNegative())); LOG_WITH_STREAM(Calc, stream << "CSSCalcValue::create from CalculationValue: " << *result); return result; }
(In reply to Antoine Quint from comment #18) > This is the first time I look at the calc() code, so it’s quite likely > there’s a better way to do this. However, it does seem like simplification > is directly related to serialization, and this looked like a good place to > do this. Another option is to simplify as we create the expression object, > but we don’t need to simplify aside from serialization, so it sounds > wasteful. Given we already simplify all calc()s that are parsed, doing it in this case doesn't sound like a problem. I think you are optimizing for a problem we don't know exists. Ifs its a perf issue here, we should do it for both cases.
(In reply to Sam Weinig from comment #22) > (In reply to Antoine Quint from comment #18) > > This is the first time I look at the calc() code, so it’s quite likely > > there’s a better way to do this. However, it does seem like simplification > > is directly related to serialization, and this looked like a good place to > > do this. Another option is to simplify as we create the expression object, > > but we don’t need to simplify aside from serialization, so it sounds > > wasteful. > > Given we already simplify all calc()s that are parsed, doing it in this case > doesn't sound like a problem. I think you are optimizing for a problem we > don't know exists. Ifs its a perf issue here, we should do it for both cases. Maybe so. But we end up creating those blended calc() values for each frame we resolve an animation that blends between mixed types. Meanwhile, serialization may never be needed.
Created attachment 425590 [details] Patch
Committed r275763 (236341@main): <https://commits.webkit.org/236341@main>
<rdar://problem/76455027>
Comment on attachment 425590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=425590&action=review > Source/WebCore/css/CSSCalculationValue.cpp:2125 > LOG_WITH_STREAM(Calc, stream << "CSSCalcValue::create from CalculationValue: " << *result); Would be nice if the logging revealed that simplification had taken place.