WebKit Bugzilla
New
Browse
Log In
×
Sign in with GitHub
or
Remember my login
Create Account
·
Forgot Password
Forgotten password account recovery
RESOLVED FIXED
158215
Make createCrossThreadTask() functions return on the stack instead of the heap
https://bugs.webkit.org/show_bug.cgi?id=158215
Summary
Make createCrossThreadTask() functions return on the stack instead of the heap
Brady Eidson
Reported
2016-05-30 15:04:19 PDT
Make createCrossThreadTask() functions return on the stack instead of the heap In our WTFMove() world there's no reason that CrossThreadTasks have to be kept in std::unique_ptrs.
Attachments
Still building the full stack locally, but I know WTF and WebCore build... this is basically an EWS run
(29.25 KB, patch)
2016-05-30 22:49 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
EWS take two
(29.25 KB, patch)
2016-05-30 22:55 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
EWS take three
(32.69 KB, patch)
2016-05-30 23:17 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(29.80 KB, patch)
2016-05-31 09:38 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(29.75 KB, patch)
2016-05-31 10:15 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Patch
(28.93 KB, patch)
2016-05-31 11:19 PDT
,
Brady Eidson
no flags
Details
Formatted Diff
Diff
Show Obsolete
(5)
View All
Add attachment
proposed patch, testcase, etc.
Brady Eidson
Comment 1
2016-05-30 22:49:57 PDT
Created
attachment 280120
[details]
Still building the full stack locally, but I know WTF and WebCore build... this is basically an EWS run
Brady Eidson
Comment 2
2016-05-30 22:55:28 PDT
Created
attachment 280121
[details]
EWS take two
Brady Eidson
Comment 3
2016-05-30 23:17:30 PDT
Created
attachment 280122
[details]
EWS take three
Darin Adler
Comment 4
2016-05-31 00:03:07 PDT
Comment on
attachment 280122
[details]
EWS take three View in context:
https://bugs.webkit.org/attachment.cgi?id=280122&action=review
I suggest we consider using Optional rather than adding a null/invalid value to CrossThreadTask.
> Source/WTF/wtf/CrossThreadQueue.h:44 > + CrossThreadQueue() > + { > + }
Could we just omit this and let it get generated? If not, I suggest using = default instead of writing an empty body.
> Source/WTF/wtf/CrossThreadQueue.h:45 > + ~CrossThreadQueue();
Could we just omit this and let it get generated?
> Source/WTF/wtf/CrossThreadQueue.h:66 > +template<typename DataType> > +inline void CrossThreadQueue<DataType>::append(DataType&& message)
Should this be inlined? Not sure it’s short enough for that to be a good idea. Also, I’ve been personally preferring a style where the template part is on the same line as the rest of the function definition. Not sure if anyone else likes that.
> Source/WTF/wtf/CrossThreadQueue.h:74 > +template<typename DataType> > +inline auto CrossThreadQueue<DataType>::waitForMessage() -> DataType
Should this be inlined? Not sure it’s short enough for that to be a good idea. We don’t need the "auto ->" syntax when it’s a template argument, as opposed to a dependent name inside the template class.
> Source/WTF/wtf/CrossThreadQueue.h:84 > + static double infiniteTime = std::numeric_limits<double>::max();
Probably should be const instead of static? Or maybe both. Is it safe to use this maximum value? Is it unsafe to use infinity instead? Can we add a waitForever function to Condition so that you don’t need to think about all these things just to read this function and know it’s correct?
> Source/WTF/wtf/CrossThreadQueue.h:92 > +template<typename DataType> > +inline auto CrossThreadQueue<DataType>::tryGetMessage() -> DataType
Should this be inlined? Not sure it’s short enough for that to be a good idea. We don’t need the "auto ->" syntax when it’s a template argument, as opposed to a dependent name inside the template class.
> Source/WTF/wtf/CrossThreadTask.h:39 > + CrossThreadTask() > + { > + }
I suggest using = default instead of writing an empty body.
> Source/WTF/wtf/CrossThreadTask.h:48 > + CrossThreadTask(CrossThreadTask&&) = default; > + CrossThreadTask& operator=(CrossThreadTask&&) = default;
Why do we need to write these? Won’t the compiler generate them?
> Source/WTF/wtf/CrossThreadTask.h:55 > + bool isValid() const { return (bool)m_taskFunction; }
Please consider static_cast instead of a C style cast. Or you could do one of those crazy !! things all the Windows folks seem to prefer.
> Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:414 > + auto task = m_mainThreadQueue.tryGetMessage(); > + while (task.isValid()) { > + task.performTask(); > + task = m_mainThreadQueue.tryGetMessage(); > + }
I would write the loop in a different way to avoid the duplication: while (true) { auto task = m_mainThreadQueue.tryGetMessage(); if (!task.isValid()) break; task.performTask(); } I wish there was a more elegant way to write it, but I think it’s worth the ugliness of "while (true)" to not repeat the tryGetMessage call. Another way to make this more elegant is to have tryGetMessage return Optional<CrossThreadTask> instead of adding an invalid state to CrossThreadTask. Then it would look like this: while (auto task = m_mainThreadQueue.tryGetMessage()) task.value().performTask();
> Source/WebCore/Modules/indexeddb/server/IDBServer.cpp:485 > + CrossThreadTask task = m_databaseReplyQueue.tryGetMessage(); > + while (task.isValid()) { > + task.performTask(); > + task = m_databaseReplyQueue.tryGetMessage(); > + }
Ditto.
Brady Eidson
Comment 5
2016-05-31 08:59:51 PDT
The Windows EWS errors make no sense to me - I'm just going to comment out CrossThreadCopier.cpp for windows.
Brady Eidson
Comment 6
2016-05-31 09:21:48 PDT
(In reply to
comment #4
)
> > > Source/WTF/wtf/CrossThreadQueue.h:84 > > + static double infiniteTime = std::numeric_limits<double>::max(); > > Probably should be const instead of static? Or maybe both. Is it safe to use > this maximum value? Is it unsafe to use infinity instead? Can we add a > waitForever function to Condition so that you don’t need to think about all > these things just to read this function and know it’s correct?
This is what MessageQueue does, so I am assuming (for now) that its safe for our needs. I think adding waitForever to condition is probably a great idea.
> > Source/WTF/wtf/CrossThreadTask.h:48 > > + CrossThreadTask(CrossThreadTask&&) = default; > > + CrossThreadTask& operator=(CrossThreadTask&&) = default; > > Why do we need to write these? Won’t the compiler generate them?
The move constructor and assignment operators are only auto generated in very specific circumstances. One of the circumstances where you do *not* get the auto generated move functions is if you have any user defined copy constructor, which this class does. More here:
http://en.cppreference.com/w/cpp/language/move_constructor
http://en.cppreference.com/w/cpp/language/move_assignment
> > Source/WTF/wtf/CrossThreadTask.h:55 > > + bool isValid() const { return (bool)m_taskFunction; } > > Please consider static_cast instead of a C style cast. Or you could do one > of those crazy !! things all the Windows folks seem to prefer.
Got rid of this for Optional, instead.
> > Source/WebCore/Modules/indexeddb/client/IDBConnectionProxy.cpp:414 > > + auto task = m_mainThreadQueue.tryGetMessage(); > > + while (task.isValid()) { > > + task.performTask(); > > + task = m_mainThreadQueue.tryGetMessage(); > > + } > > I would write the loop in a different way to avoid the duplication: > > while (true) { > auto task = m_mainThreadQueue.tryGetMessage(); > if (!task.isValid()) > break; > task.performTask(); > } > > I wish there was a more elegant way to write it, but I think it’s worth the > ugliness of "while (true)" to not repeat the tryGetMessage call.
I actually did that first, and then though quite specifically - "Darin hates while(true)"
> > Another way to make this more elegant is to have tryGetMessage return > Optional<CrossThreadTask> instead of adding an invalid state to > CrossThreadTask. Then it would look like this: > > while (auto task = m_mainThreadQueue.tryGetMessage()) > task.value().performTask();
That's what we have now.
Brady Eidson
Comment 7
2016-05-31 09:38:34 PDT
Created
attachment 280151
[details]
Patch
Darin Adler
Comment 8
2016-05-31 09:52:49 PDT
Comment on
attachment 280151
[details]
Patch View in context:
https://bugs.webkit.org/attachment.cgi?id=280151&action=review
> Source/WTF/wtf/CrossThreadCopier.cpp:37 > +#if !OS(WINDOWS)
Would be nice to have a comment explaining why we don’t even try to compile these tests on Windows.
> Source/WTF/wtf/CrossThreadQueue.h:93 > +template<typename DataType> > +Optional<DataType> CrossThreadQueue<DataType>::tryGetMessage() > +{ > + LockHolder lock(m_lock); > + > + if (m_queue.isEmpty()) > + return { }; > + > + return m_queue.takeFirst(); > +}
I don’t think anyone is using this function.
> Source/WTF/wtf/CrossThreadTask.h:46 > + CrossThreadTask(CrossThreadTask&&) = default; > + CrossThreadTask& operator=(CrossThreadTask&&) = default;
I think we only have to explicitly define these because we use WTF_MAKE_NONCOPYABLE. Since NoncopyableFunction is already non-copyable, I think we can get the same results by leaving out these two lines and also removing the use of WTF_MAKE_NONCOPYABLE on this class.
Brady Eidson
Comment 9
2016-05-31 10:15:43 PDT
Created
attachment 280155
[details]
Patch
Brady Eidson
Comment 10
2016-05-31 10:17:05 PDT
Try to get Windows happy.
WebKit Commit Bot
Comment 11
2016-05-31 10:17:48 PDT
Attachment 280155
[details]
did not pass style-queue: ERROR: Source/WTF/wtf/CrossThreadCopier.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
Brady Eidson
Comment 12
2016-05-31 10:23:10 PDT
(In reply to
comment #8
)
> Comment on
attachment 280151
[details]
> Patch > > View in context: >
https://bugs.webkit.org/attachment.cgi?id=280151&action=review
> > > Source/WTF/wtf/CrossThreadCopier.cpp:37 > > +#if !OS(WINDOWS) > > Would be nice to have a comment explaining why we don’t even try to compile > these tests on Windows.
Trying a different fix based on consulting with Alex.
> > > Source/WTF/wtf/CrossThreadQueue.h:93 > > +template<typename DataType> > > +Optional<DataType> CrossThreadQueue<DataType>::tryGetMessage() > > +{ > > + LockHolder lock(m_lock); > > + > > + if (m_queue.isEmpty()) > > + return { }; > > + > > + return m_queue.takeFirst(); > > +} > > I don’t think anyone is using this function.
It is supposed to be! I must've done something tragically wrong in this iteration, will go find.
> > > Source/WTF/wtf/CrossThreadTask.h:46 > > + CrossThreadTask(CrossThreadTask&&) = default; > > + CrossThreadTask& operator=(CrossThreadTask&&) = default; > > I think we only have to explicitly define these because we use > WTF_MAKE_NONCOPYABLE. Since NoncopyableFunction is already non-copyable, I > think we can get the same results by leaving out these two lines and also > removing the use of WTF_MAKE_NONCOPYABLE on this class.
You may be right! I'll try that.
Brady Eidson
Comment 13
2016-05-31 11:12:21 PDT
(In reply to
comment #12
)
> (In reply to
comment #8
) > > > Source/WTF/wtf/CrossThreadQueue.h:93 > > > +template<typename DataType> > > > +Optional<DataType> CrossThreadQueue<DataType>::tryGetMessage() > > > +{ > > > + LockHolder lock(m_lock); > > > + > > > + if (m_queue.isEmpty()) > > > + return { }; > > > + > > > + return m_queue.takeFirst(); > > > +} > > > > I don’t think anyone is using this function. > > It is supposed to be! I must've done something tragically wrong in this > iteration, will go find.
It *is* used - code that used to use tryGetMessage on MessageQueue now uses it on CrossThreadQueue. Those code sites reverted to untouched (after your comments on EWS take three), so they don't show in the patch.
Brady Eidson
Comment 14
2016-05-31 11:12:45 PDT
Windows bot made much more progress this time, suggesting it's over its WTF hurdle. \o/
Brady Eidson
Comment 15
2016-05-31 11:17:21 PDT
(In reply to
comment #12
)
> (In reply to
comment #8
) > > > > > Source/WTF/wtf/CrossThreadTask.h:46 > > > + CrossThreadTask(CrossThreadTask&&) = default; > > > + CrossThreadTask& operator=(CrossThreadTask&&) = default; > > > > I think we only have to explicitly define these because we use > > WTF_MAKE_NONCOPYABLE. Since NoncopyableFunction is already non-copyable, I > > think we can get the same results by leaving out these two lines and also > > removing the use of WTF_MAKE_NONCOPYABLE on this class. > > You may be right! I'll try that.
Haven't 100% finished my build yet, but it looks like this worked.
Brady Eidson
Comment 16
2016-05-31 11:19:40 PDT
Created
attachment 280159
[details]
Patch
Brady Eidson
Comment 17
2016-05-31 11:20:25 PDT
I'll wait for a full EWS sign off before cq+'ing this.
WebKit Commit Bot
Comment 18
2016-05-31 11:22:40 PDT
Attachment 280159
[details]
did not pass style-queue: ERROR: Source/WTF/wtf/CrossThreadCopier.cpp:33: Found other header before a header this file implements. Should be: config.h, primary header, blank line, and then alphabetically sorted. [build/include_order] [4] Total errors found: 1 in 16 files If any of these errors are false positives, please file a bug against check-webkit-style.
WebKit Commit Bot
Comment 19
2016-05-31 12:32:05 PDT
Comment on
attachment 280159
[details]
Patch Clearing flags on attachment: 280159 Committed
r201518
: <
http://trac.webkit.org/changeset/201518
>
WebKit Commit Bot
Comment 20
2016-05-31 12:32:13 PDT
All reviewed patches have been landed. Closing bug.
Note
You need to
log in
before you can comment on or make changes to this bug.
Top of Page
Format For Printing
XML
Clone This Bug