Bug 158367 – Avoid an extra heap allocation when dispatching Functions to WorkQueue

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

[Anders Carlsson]

Avoid an extra heap allocation when dispatching Functions to WorkQueue

[Anders Carlsson]

Created attachment 280479 [details]
Patch

[WebKit Commit Bot]

Attachment 280479 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/Function.h:73:  Boolean expressions that span multiple lines should have their operators on the left side of the line instead of the right side.  [whitespace/operators] [4]
ERROR: Source/WTF/wtf/Function.h:74:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Comment on attachment 280479 [details]
Patch

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

> Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:37
> +        callback.callback(callback.context);

Cannot we make it look like:
callback(); ?

It is a bit sad to have to specify callback.context at the call site, or having callback.callback.

[Chris Dumez]

Comment on attachment 280479 [details]
Patch

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

> Source/WTF/wtf/Function.h:75
> +        return Callback { m_callableWrapper.release(), [](void* context, In... in) -> Out {

Wouldn't this work?:
Auto* contextPtr = m_callableWrapper.get();
return Callback { m_callableWrapper.release(), [contextPtr](In... in) -> Out {
...
};

[Chris Dumez]

Comment on attachment 280479 [details]
Patch

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

>> Source/WTF/wtf/Function.h:75
>> +        return Callback { m_callableWrapper.release(), [](void* context, In... in) -> Out {
> 
> Wouldn't this work?:
> Auto* contextPtr = m_callableWrapper.get();
> return Callback { m_callableWrapper.release(), [contextPtr](In... in) -> Out {
> ...
> };

And then Callback's members would be private and it would have an operator()(In...).

[Anders Carlsson]

(In reply to comment #4)
> Comment on attachment 280479 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=280479&action=review
> 
> > Source/WTF/wtf/Function.h:75
> > +        return Callback { m_callableWrapper.release(), [](void* context, In... in) -> Out {
> 
> Wouldn't this work?:
> Auto* contextPtr = m_callableWrapper.get();
> return Callback { m_callableWrapper.release(), [contextPtr](In... in) -> Out
> {
> ...
> };

It wouldn't work, because the second lambda is just a function declaration - it can't capture anything.

[Chris Dumez]

Comment on attachment 280479 [details]
Patch

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

>>>> Source/WTF/wtf/Function.h:75
>>>> +        return Callback { m_callableWrapper.release(), [](void* context, In... in) -> Out {
>>> 
>>> Wouldn't this work?:
>>> Auto* contextPtr = m_callableWrapper.get();
>>> return Callback { m_callableWrapper.release(), [contextPtr](In... in) -> Out {
>>> ...
>>> };
>> 
>> And then Callback's members would be private and it would have an operator()(In...).
> 
> It wouldn't work, because the second lambda is just a function declaration - it can't capture anything.

Oh, I see. I still worry about several things:
1. It seems a bit leak-prone. We'll leak if no one calls callback().
2. It seems a bit error-prone. It will crash if someone tries to call the callback more than once
3. The syntax is not very nice at the call site.

[Chris Dumez]

Created attachment 281672 [details]
Alternative proposal

Anders, what would you think about something like this?

[WebKit Commit Bot]

Attachment 281672 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:47:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Created attachment 281677 [details]
Alternative proposal

[WebKit Commit Bot]

Attachment 281677 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:47:  Extra space before ( in function call  [whitespace/parens] [4]
Total errors found: 2 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Anders, any thoughts?

[Chris Dumez]

Ping?

[Anders Carlsson]

Comment on attachment 281677 [details]
Alternative proposal

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

> Source/WTF/wtf/NoncopyableFunction.h:77
> +    CallableWrapperBase* leakCallable()

Should WARN_UNUSED_RESULT here (if possible).

[Chris Dumez]

Created attachment 283667 [details]
Patch

[WebKit Commit Bot]

Attachment 283667 [details] did not pass style-queue:


ERROR: Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:36:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/cocoa/WorkQueueCocoa.cpp:47:  Extra space before ( in function call  [whitespace/parens] [4]
ERROR: Source/WTF/wtf/Function.h:78:  This { should be at the end of the previous line  [whitespace/braces] [4]
Total errors found: 3 in 3 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Chris Dumez]

Committed r203236: <http://trac.webkit.org/changeset/203236>