Bug 224586 – Use WTF::Function instead of std::function in SVGPropertyAnimatorFactory::attributeAnimatorCreator

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Alex Christensen]

Use WTF::Function instead of std::function in SVGPropertyAnimatorFactory::attributeAnimatorCreator

[Alex Christensen]

Created attachment 426061 [details]
Patch

[Geoffrey Garen]

Comment on attachment 426061 [details]
Patch

EWS too angry

[Alex Christensen]

Created attachment 426068 [details]
Patch

[Darin Adler]

Comment on attachment 426068 [details]
Patch

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

> Source/WebCore/ChangeLog:9
> +        sizeof(WTF::Function<void()>) is 8.
> +        sizeof(std::function<void()>) is 48.

What about total memory use? I’m wondering if we should even allow std::function if it uses more memory, but maybe this is more about "what's in the top level of the object and what's on the heap"?

> Source/WebCore/svg/properties/SVGPropertyAnimatorFactory.h:112
> +            Pair { SVGNames::colorAttr->impl(),              std::make_pair(SVGValueProperty<Color>::create, SVGPropertyAnimatorFactory::createColorAnimator) },

Is there a good reason we are using std::make_pair instead of just brace syntax? Could we make this even tighter?

[Chris Dumez]

(In reply to Darin Adler from comment #4)
> Comment on attachment 426068 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=426068&action=review
> 
> > Source/WebCore/ChangeLog:9
> > +        sizeof(WTF::Function<void()>) is 8.
> > +        sizeof(std::function<void()>) is 48.
> 
> What about total memory use? I’m wondering if we should even allow
> std::function if it uses more memory, but maybe this is more about "what's
> in the top level of the object and what's on the heap"?
> 
> > Source/WebCore/svg/properties/SVGPropertyAnimatorFactory.h:112
> > +            Pair { SVGNames::colorAttr->impl(),              std::make_pair(SVGValueProperty<Color>::create, SVGPropertyAnimatorFactory::createColorAnimator) },
> 
> Is there a good reason we are using std::make_pair instead of just brace
> syntax? Could we make this even tighter?

My understanding is that std::function tries to be smart about when to store the data inline or do a heap allocation. It makes this determination based on the size of the data. WTF::Function always does a heap allocation no matter the size of the data.

[Alex Christensen]

Created attachment 426112 [details]
Patch

[Alex Christensen]

Comment on attachment 426112 [details]
Patch

I'm not sure about heap/inline memory use total, but I do know that inside a HashMap we want inline to be smaller.

[Alex Christensen]

r276033

[Darin Adler]

(In reply to Alex Christensen from comment #7)
> I'm not sure about heap/inline memory use total, but I do know that inside a
> HashMap we want inline to be smaller.

Sure, but at a certain point for hash tables we will do that by putting the whole thing on the heap, not by building the capability into classes.

For something we are using as widely as WTF::Function our lens should be broader than just suitability in hash tables.