Bug 210138 – [Cocoa] Simplify NSArray, NSDictionary, and NSNumber idioms throughout WebKit

• Component: WebKit Misc.
• Status: RESOLVED FIXED

[Darin Adler]

[Cocoa] Simplify NSArray and NSDictionary idioms throughout WebKit

[Darin Adler]

Created attachment 395769 [details]
Patch

[Darin Adler]

Believe it or not, this started as a live range patch, but I ended up going down a rabbit hole of Cocoa-specific stuff.

[Sam Weinig]

Comment on attachment 395769 [details]
Patch

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

> Source/WTF/wtf/cocoa/VectorCocoa.h:51
> +template<typename VectorType> RetainPtr<NSArray> createNSArray(const VectorType& vector)

I really like this. Hated copying this idiom all over. How would you feel about an overload that takes a lambda for one off cases such as the ones in PlatformCAAnimationCocoa?

Also, why the discrepency in naming prefixes here? *create*NSArray vs. *make*Vector?

> Source/WebCore/platform/graphics/FloatRect.h:283
> +WEBCORE_EXPORT id makeNSArrayElement(const FloatRect&);

The makeNSArrayElement() for String (and the description in VectorCocoa.h) return RetainPtr<id>. Should this be consistent?

> Source/WebCore/platform/graphics/IntRect.h:254
> +WEBCORE_EXPORT id makeNSArrayElement(const IntRect&);

The makeNSArrayElement() for String (and the description in VectorCocoa.h) return RetainPtr<id>. Should this be consistent?

[Darin Adler]

Comment on attachment 395769 [details]
Patch

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

>> Source/WTF/wtf/cocoa/VectorCocoa.h:51
>> +template<typename VectorType> RetainPtr<NSArray> createNSArray(const VectorType& vector)
> 
> I really like this. Hated copying this idiom all over. How would you feel about an overload that takes a lambda for one off cases such as the ones in PlatformCAAnimationCocoa?
> 
> Also, why the discrepency in naming prefixes here? *create*NSArray vs. *make*Vector?

The overload with lambda sounds great. I will add it and use it in a future patch.

I started with the names makeNSArray and makeVector and would be willing to go back. I renamed makeNSArray to createNSArray because of our tradition of using the word "create" for functions that return a new object in a smart pointer. Happy to do global replace on the name either before landing or after.

>> Source/WebCore/platform/graphics/FloatRect.h:283
>> +WEBCORE_EXPORT id makeNSArrayElement(const FloatRect&);
> 
> The makeNSArrayElement() for String (and the description in VectorCocoa.h) return RetainPtr<id>. Should this be consistent?

This is my attempt at a possibly-unimportant optimization. The idea is that makeNSArrayElement can either return an autoreleased object in an id or a retained object in a RetainPtr<id>. Always returning RetainPtr<id> would require a little bit more reference count churn in an autoreleased case like this one. The distinction would disappear if we ever move to ARC, of course.

I probably should have documented this in the comment in VectorCocoa.h -- by the time I was writing that I had forgotten about it!

[Sam Weinig]

(In reply to Darin Adler from comment #4)
> Comment on attachment 395769 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=395769&action=review
> 
> >> Source/WTF/wtf/cocoa/VectorCocoa.h:51
> >> +template<typename VectorType> RetainPtr<NSArray> createNSArray(const VectorType& vector)
> > 
> > I really like this. Hated copying this idiom all over. How would you feel about an overload that takes a lambda for one off cases such as the ones in PlatformCAAnimationCocoa?
> > 
> > Also, why the discrepency in naming prefixes here? *create*NSArray vs. *make*Vector?
> 
> The overload with lambda sounds great. I will add it and use it in a future
> patch.
> 
> I started with the names makeNSArray and makeVector and would be willing to
> go back. I renamed makeNSArray to createNSArray because of our tradition of
> using the word "create" for functions that return a new object in a smart
> pointer. Happy to do global replace on the name either before landing or
> after.

I think that makes sense. No need to change it.

> 
> >> Source/WebCore/platform/graphics/FloatRect.h:283
> >> +WEBCORE_EXPORT id makeNSArrayElement(const FloatRect&);
> > 
> > The makeNSArrayElement() for String (and the description in VectorCocoa.h) return RetainPtr<id>. Should this be consistent?
> 
> This is my attempt at a possibly-unimportant optimization. The idea is that
> makeNSArrayElement can either return an autoreleased object in an id or a
> retained object in a RetainPtr<id>. Always returning RetainPtr<id> would
> require a little bit more reference count churn in an autoreleased case like
> this one. The distinction would disappear if we ever move to ARC, of course.
> 
> I probably should have documented this in the comment in VectorCocoa.h -- by
> the time I was writing that I had forgotten about it!

Yeah, I think just updating the comment in VectorCocoa.h is all that needs to change here.

[Darin Adler]

Created attachment 395857 [details]
Patch

[Darin Adler]

New patch fixes the NSGeometry problem that was breaking the build and addresses Sam's comment about the VectorCocoa.h comment.

[Darin Adler]

Created attachment 395864 [details]
Patch

[Darin Adler]

Created attachment 395873 [details]
Patch

[Darin Adler]

Created attachment 395894 [details]
Patch

[Darin Adler]

Created attachment 395905 [details]
Patch

[Darin Adler]

Finally, tests all passing. Sam, would you be willing to review?

[Darin Adler]

Comment on attachment 395905 [details]
Patch

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

> Source/WTF/wtf/cocoa/VectorCocoa.h:77
> +using WTF::makeVector;

Note, the reason I don’t need to put a "using WTF::createNSArray" here is that we are calling it with WTF::Vector objects, so argument-dependent lookup will find createNSArray. If we ever wanted to use it on objects of another type that just happened to have both a size function and were compatible with range-based for loops, but were not in the WTF namespace, then we could add "WTF::createNSArray" and it would work just as well with them.

Or I could just add the "using" line and not let things be so subtle.

[Alex Christensen]

Comment on attachment 395905 [details]
Patch

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

> Source/WTF/wtf/cocoa/VectorCocoa.h:39
> +// if the value is autoreleased. The makeVectorElement function takes an ignored
> +// pointer to the vector element type, making argument-dependent lookup work, and an

Doesn't this still require putting a pointer into a certain register?  It seems like it would be even faster if we would just use a template type and template overloads instead of an unused null typed pointer.

[Darin Adler]

Comment on attachment 395905 [details]
Patch

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

>> Source/WTF/wtf/cocoa/VectorCocoa.h:39
>> +// pointer to the vector element type, making argument-dependent lookup work, and an
> 
> Doesn't this still require putting a pointer into a certain register?  It seems like it would be even faster if we would just use a template type and template overloads instead of an unused null typed pointer.

I don’t think it would be measurably faster.

- If the function is inlined, then no, no pointer ends up being put into a register.
- If the function is not inlined, then putting a zero into a register is super-low-cost.

Using a template (the traits pattern) requires messy things like specializing in the namespace that the template is in. And also requires that the functions be in headers rather than .cpp files. I’m not sure it’s the right way to go.

We can get the efficiency benefit from just inlining the makeVectorElement function.

What do you think?

[Alex Christensen]

If makeVectorElement is always inline, then it will always be super easy for the compiler to optimize out the nullptr entirely.

[Darin Adler]

It’s not required to always be inline, but if we want better performance, I suggest that be the fix.

But I’m also willing to consider other designs. The number of makeVectorElement functions is very small compared to the number of makeVector call sites, so changes to makeVectorElement would be easy to do.

[Darin Adler]

Thanks for the comment, by the way. I’m willing to make changes.

Still looking for a reviewer, in case you’d like be willing to look over the rest of the patch.

[Alex Christensen]

Comment on attachment 395905 [details]
Patch

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

> Source/WTF/wtf/cocoa/VectorCocoa.h:69
> +        constexpr const VectorElementType* typedNull = nullptr;
> +        if (auto vectorElement = makeVectorElement(typedNull, element))

I think all this is great.  It's a wonderful improvement to our code.  I think it would be even better if it looked like this:
if (auto vectorElement = makeVectorElement<VectorElementType>(element))
I think we could also do that later if we decide that's indeed better.  This isn't bad either.

[Sam Weinig]

Comment on attachment 395905 [details]
Patch

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

r=me as well.

>>> Source/WTF/wtf/cocoa/VectorCocoa.h:39
>>> +// pointer to the vector element type, making argument-dependent lookup work, and an
>> 
>> Doesn't this still require putting a pointer into a certain register?  It seems like it would be even faster if we would just use a template type and template overloads instead of an unused null typed pointer.
> 
> I don’t think it would be measurably faster.
> 
> - If the function is inlined, then no, no pointer ends up being put into a register.
> - If the function is not inlined, then putting a zero into a register is super-low-cost.
> 
> Using a template (the traits pattern) requires messy things like specializing in the namespace that the template is in. And also requires that the functions be in headers rather than .cpp files. I’m not sure it’s the right way to go.
> 
> We can get the efficiency benefit from just inlining the makeVectorElement function.
> 
> What do you think?

I think it is very unlikely the function won't be inlined in release code, so the cost should be zero as you say.

[Darin Adler]

Committed r259843: <https://trac.webkit.org/changeset/259843>

[Radar WebKit Bug Importer]

<rdar://problem/61557276>