Make createCrossThreadTask() functions return on the stack instead of the heap

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Brady Eidson]

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.

[Brady Eidson]

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]

Created attachment 280121 [details]
EWS take two

[Brady Eidson]

Created attachment 280122 [details]
EWS take three

[Darin Adler]

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]

The Windows EWS errors make no sense to me - I'm just going to comment out CrossThreadCopier.cpp for windows.

[Brady Eidson]

(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]

Created attachment 280151 [details]
Patch

[Darin Adler]

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]

Created attachment 280155 [details]
Patch

[Brady Eidson]

Try to get Windows happy.

[WebKit Commit Bot]

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]

(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]

(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]

Windows bot made much more progress this time, suggesting it's over its WTF hurdle.

\o/

[Brady Eidson]

(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]

Created attachment 280159 [details]
Patch

[Brady Eidson]

I'll wait for a full EWS sign off before cq+'ing this.

[WebKit Commit Bot]

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 on attachment 280159 [details]
Patch

Clearing flags on attachment: 280159

Committed r201518: <http://trac.webkit.org/changeset/201518>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.