[webkit-reviews] review granted: [Bug 224169] Wasted vector capacity in FEColorMatrix and filters : [Attachment 425123] Patch

[bugzilla-daemon at webkit.org]

Said Abou-Hallawa <sabouhallawa at apple.com> has granted Simon Fraser (smfr)
<simon.fraser at apple.com>'s request for review:
Bug 224169: Wasted vector capacity in FEColorMatrix and filters
https://bugs.webkit.org/show_bug.cgi?id=224169

Attachment 425123: Patch

https://bugs.webkit.org/attachment.cgi?id=425123&action=review




--- Comment #3 from Said Abou-Hallawa <sabouhallawa at apple.com> ---
Comment on attachment 425123
  --> https://bugs.webkit.org/attachment.cgi?id=425123
Patch

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

> Source/WebCore/ChangeLog:8
> +	   Shrink the vector of input effects in FilterEffect.

I think we can reserveInitialCapacity the inputEffects for all FilterEffects
instead of shrinking them later. Also do not we care about shrinking
CSSFilter::m_effects?

> Source/WebCore/rendering/CSSFilter.cpp:299
>		   effect->inputEffects().append(WTFMove(previousEffect));

I think this can be written like this:

    effect->inputEffects().reserveInitialCapacity(1);
    effect->inputEffects().uncheckedAppend(WTFMove(previousEffect));

Or
    effect->inputEffects() = { WTFMove(previousEffect) };

And we do not need to call didFinishBuilding().

> Source/WebCore/rendering/CSSFilter.cpp:303
> +	       effect->didFinishBuilding();

Why do we need to call didFinishBuilding() for the case filterOperation.type()
== FilterOperation::REFERENCE? CSSFilter::buildReferenceFilter() already calls
didFinishBuilding() for all the effects including the last one which it returns
and which is stored in "effect".

> Source/WebCore/rendering/svg/RenderSVGResourceFilter.cpp:108
> +	   effect->didFinishBuilding();

Why don't we let the SVG FilterEffect elements reserveInitialCapacity the
inputEffects before appending to it instead of shrinking it here? I think all
of them, except SVGFEMergeElement, have a fixed number of inputEffects. For
example in SVGFEGaussianBlurElement::build() we can do this:

    auto effect = FEGaussianBlur::create(filter, stdDeviationX(),
stdDeviationY(), edgeMode());
    effect->inputEffects().reserveInitialCapacity(1);
    effect->inputEffects().uncheckedAppend(WTFMove(input1));

Or 

    auto effect = FEGaussianBlur::create(filter, stdDeviationX(),
stdDeviationY(), edgeMode());
    effect->inputEffects() = { WTFMove(input1) };

> Source/WebCore/svg/SVGFEColorMatrixElement.cpp:141
>	   filterValues = values();
> +	   filterValues.shrinkToFit();

Can this be written like this?
    filterValues.reserveInitialCapacity(values().size());
    filterValues.appendVector(values());