[Cocoa] Simplify NSArray and NSDictionary idioms throughout WebKit
Created attachment 395769 [details] Patch
Believe it or not, this started as a live range patch, but I ended up going down a rabbit hole of Cocoa-specific stuff.
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?
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!
(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.
Created attachment 395857 [details] Patch
New patch fixes the NSGeometry problem that was breaking the build and addresses Sam's comment about the VectorCocoa.h comment.
Created attachment 395864 [details] Patch
Created attachment 395873 [details] Patch
Created attachment 395894 [details] Patch
Created attachment 395905 [details] Patch
Finally, tests all passing. Sam, would you be willing to review?
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.
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.
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?
If makeVectorElement is always inline, then it will always be super easy for the compiler to optimize out the nullptr entirely.
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.
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.
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.
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.
Committed r259843: <https://trac.webkit.org/changeset/259843>
<rdar://problem/61557276>