Bug 224169

Summary: Wasted vector capacity in FEColorMatrix and filters
Product: WebKit Reporter: Simon Fraser (smfr) <simon.fraser>
Component: SVGAssignee: Simon Fraser (smfr) <simon.fraser>
Status: RESOLVED FIXED    
Severity: Normal CC: changseok, dino, esprehn+autocc, ews-watchlist, fmalita, glenn, gyuyoung.kim, kondapallykalyan, pdr, sabouhallawa, schenney, sergio, simon.fraser, webkit-bug-importer, zimmermann
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Patch
none
Patch none

Description Simon Fraser (smfr) 2021-04-03 20:55:57 PDT
Wasted vector capacit in FEColorMatrix and filters
Comment 1 Simon Fraser (smfr) 2021-04-03 20:57:37 PDT
Created attachment 425123 [details]
Patch
Comment 2 Simon Fraser (smfr) 2021-04-03 20:58:02 PDT
*** Bug 224168 has been marked as a duplicate of this bug. ***
Comment 3 Said Abou-Hallawa 2021-04-04 01:51:05 PDT
Comment on attachment 425123 [details]
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());
Comment 4 Simon Fraser (smfr) 2021-04-05 11:18:28 PDT
Created attachment 425176 [details]
Patch
Comment 5 EWS 2021-04-05 18:45:45 PDT
Committed r275471: <https://commits.webkit.org/r275471>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 425176 [details].
Comment 6 Radar WebKit Bug Importer 2021-04-05 18:51:14 PDT
<rdar://problem/76247318>