Bug 163615

Created attachment 291980 [details] maybe this will work

Created attachment 292029 [details] the patch

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/

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

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.

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

Created attachment 292044 [details] patch for landing

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.

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.

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

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