Bug 224169 - Wasted vector capacity in FEColorMatrix and filters
Summary: Wasted vector capacity in FEColorMatrix and filters
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: SVG (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Simon Fraser (smfr)
URL:
Keywords: InRadar
: 224168 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-04-03 20:55 PDT by Simon Fraser (smfr)
Modified: 2021-04-05 18:51 PDT (History)
15 users (show)

See Also:


Attachments
Patch (16.69 KB, patch)
2021-04-03 20:57 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff
Patch (22.48 KB, patch)
2021-04-05 11:18 PDT, Simon Fraser (smfr)
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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>