Bug 163615 – DFG worklist should use AutomaticThread

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Filip Pizlo]

Patch forthcoming.

[Filip Pizlo]

Created attachment 291980 [details]
maybe this will work

[Filip Pizlo]

Created attachment 292029 [details]
the patch

[Mark Lam]

Comment on attachment 292029 [details]
the patch

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

r=me

> Source/JavaScriptCore/dfg/DFGWorklist.cpp:78
> +                m_plan = nullptr;

I think it might be cleaner to use an RAII object to (1) RELEASE_ASSERT(m_plan) above, and (2) nullify m_plan on return.

> Source/WTF/wtf/AutomaticThread.h:150
> +    // It's sometimes useful to allocate resources while the thread is running, and do destroy them

typo: /do destroy/to destroy/

[Filip Pizlo]

(In reply to comment #3)
> Comment on attachment 292029 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292029&action=review
> 
> r=me
> 
> > Source/JavaScriptCore/dfg/DFGWorklist.cpp:78
> > +                m_plan = nullptr;
> 
> I think it might be cleaner to use an RAII object to (1)
> RELEASE_ASSERT(m_plan) above, and (2) nullify m_plan on return.

I agree.  I will do that.

Also, I can use that scope to decrement m_numberOfActiveThreads.

> 
> > Source/WTF/wtf/AutomaticThread.h:150
> > +    // It's sometimes useful to allocate resources while the thread is running, and do destroy them
> 
> typo: /do destroy/to destroy/

Fixed.

[Filip Pizlo]

Comment on attachment 292029 [details]
the patch

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

>>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:78
>>> +                m_plan = nullptr;
>> 
>> I think it might be cleaner to use an RAII object to (1) RELEASE_ASSERT(m_plan) above, and (2) nullify m_plan on return.
> 
> I agree.  I will do that.
> 
> Also, I can use that scope to decrement m_numberOfActiveThreads.

Hmmm, but that requires some tricks - since that count must be decremented when the lock is held.  I'll come up with something.

[Filip Pizlo]

(In reply to comment #5)
> Comment on attachment 292029 [details]
> the patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=292029&action=review
> 
> >>> Source/JavaScriptCore/dfg/DFGWorklist.cpp:78
> >>> +                m_plan = nullptr;
> >> 
> >> I think it might be cleaner to use an RAII object to (1) RELEASE_ASSERT(m_plan) above, and (2) nullify m_plan on return.
> > 
> > I agree.  I will do that.
> > 
> > Also, I can use that scope to decrement m_numberOfActiveThreads.
> 
> Hmmm, but that requires some tricks - since that count must be decremented
> when the lock is held.  I'll come up with something.

Oh it's easy!  The m_numberOfActiveThreads count is just for debugging.  So, although we want to hold the lock when decrementing it, we can do it in a separate critical section from everything else.  This means that the scoped idiom still works.

[Filip Pizlo]

Created attachment 292044 [details]
patch for landing

[Filip Pizlo]

I'm going to build the full stack so that I can run JetStream locally.  I'm pretty sure that JetStream might thrash with the 1 second timeout.  My hope is that it won't matter because the cost of starting a thread is low enough that you can safely pay it once per second.

[WebKit Commit Bot]

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


ERROR: Source/JavaScriptCore/dfg/DFGWorklist.cpp:112:  Multi line control clauses should use braces.  [whitespace/braces] [4]
Total errors found: 1 in 8 files


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

[Filip Pizlo]

(In reply to comment #8)
> I'm going to build the full stack so that I can run JetStream locally.  I'm
> pretty sure that JetStream might thrash with the 1 second timeout.  My hope
> is that it won't matter because the cost of starting a thread is low enough
> that you can safely pay it once per second.

My experiments show that this patch might be a small JetStream progression.

[Filip Pizlo]

Landed in https://trac.webkit.org/changeset/207545