Bug 158367 - Avoid an extra heap allocation when dispatching Functions to WorkQueue
Summary: Avoid an extra heap allocation when dispatching Functions to WorkQueue
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-06-03 16:07 PDT by Anders Carlsson
Modified: 2022-02-28 03:50 PST (History)
6 users (show)

See Also:


Attachments
Patch (3.06 KB, patch)
2016-06-03 16:10 PDT, Anders Carlsson
no flags Details | Formatted Diff | Diff
Alternative proposal (2.11 KB, patch)
2016-06-20 13:25 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Alternative proposal (2.91 KB, patch)
2016-06-20 14:15 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (2.99 KB, patch)
2016-07-14 12:26 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Anders Carlsson 2016-06-03 16:07:37 PDT
Avoid an extra heap allocation when dispatching Functions to WorkQueue
Comment 1 Anders Carlsson 2016-06-03 16:10:12 PDT
Created attachment 280479 [details]
Patch
Comment 2 WebKit Commit Bot 2016-06-03 16:12:31 PDT
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.
Comment 3 Chris Dumez 2016-06-03 16:21:32 PDT
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.
Comment 4 Chris Dumez 2016-06-03 16:23:23 PDT
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 {
...
};
Comment 5 Chris Dumez 2016-06-03 16:24:24 PDT
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...).
Comment 6 Anders Carlsson 2016-06-03 16:26:43 PDT
(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.
Comment 7 Chris Dumez 2016-06-06 09:07:49 PDT
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.
Comment 8 Chris Dumez 2016-06-20 13:25:16 PDT
Created attachment 281672 [details]
Alternative proposal

Anders, what would you think about something like this?
Comment 9 WebKit Commit Bot 2016-06-20 13:27:04 PDT
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.
Comment 10 Chris Dumez 2016-06-20 14:15:11 PDT
Created attachment 281677 [details]
Alternative proposal
Comment 11 WebKit Commit Bot 2016-06-20 14:16:06 PDT
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.
Comment 12 Chris Dumez 2016-06-22 09:21:51 PDT
Anders, any thoughts?
Comment 13 Chris Dumez 2016-07-06 14:50:38 PDT
Ping?
Comment 14 Anders Carlsson 2016-07-14 12:19:02 PDT
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).
Comment 15 Chris Dumez 2016-07-14 12:26:36 PDT
Created attachment 283667 [details]
Patch
Comment 16 WebKit Commit Bot 2016-07-14 12:27:56 PDT
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.
Comment 17 Chris Dumez 2016-07-14 12:46:17 PDT
Committed r203236: <http://trac.webkit.org/changeset/203236>