RESOLVED FIXED 119833
Concurrent compilation thread should not trigger WriteBarriers
https://bugs.webkit.org/show_bug.cgi?id=119833
Summary Concurrent compilation thread should not trigger WriteBarriers
Mark Hahnenberg
Reported 2013-08-14 19:32:47 PDT
The concurrent compilation should interact minimally with the Heap, including triggering WriteBarriers. This is a prerequisite for generational GC.
Attachments
Patch (44.99 KB, patch)
2013-08-14 21:25 PDT, Mark Hahnenberg
no flags
Patch (47.18 KB, patch)
2013-08-14 21:38 PDT, Mark Hahnenberg
no flags
Patch (47.19 KB, patch)
2013-08-15 07:42 PDT, Mark Hahnenberg
no flags
Patch (46.29 KB, patch)
2013-08-15 14:02 PDT, Mark Hahnenberg
no flags
Mark Hahnenberg
Comment 1 2013-08-14 19:33:23 PDT
The concurrent compilation thread*
Mark Hahnenberg
Comment 2 2013-08-14 21:25:10 PDT
Early Warning System Bot
Comment 3 2013-08-14 21:35:09 PDT
Early Warning System Bot
Comment 4 2013-08-14 21:36:26 PDT
Mark Hahnenberg
Comment 5 2013-08-14 21:38:19 PDT
EFL EWS Bot
Comment 6 2013-08-14 22:13:02 PDT
EFL EWS Bot
Comment 7 2013-08-14 22:28:12 PDT
Mark Hahnenberg
Comment 8 2013-08-15 07:42:02 PDT
Oliver Hunt
Comment 9 2013-08-15 09:17:55 PDT
Why shouldn't it trigger a write barrier? If the concurrent hit puts a reference to a young generation object into an old generation object it needs to trigger the barrier. So is it not possible for this to happen? Or do we have a delayed trigger?
Mark Hahnenberg
Comment 10 2013-08-15 10:10:14 PDT
(In reply to comment #9) > Why shouldn't it trigger a write barrier? > > If the concurrent hit puts a reference to a young generation object into an old generation object it needs to trigger the barrier. So is it not possible for this to happen? Or do we have a delayed trigger? Delayed.
Oliver Hunt
Comment 11 2013-08-15 10:39:02 PDT
(In reply to comment #10) > (In reply to comment #9) > > Why shouldn't it trigger a write barrier? > > > > If the concurrent hit puts a reference to a young generation object into an old generation object it needs to trigger the barrier. So is it not possible for this to happen? Or do we have a delayed trigger? > > Delayed. What keeps young objects live when they're only in the deferred set?
Mark Hahnenberg
Comment 12 2013-08-15 10:39:47 PDT
(In reply to comment #11) > (In reply to comment #10) > > (In reply to comment #9) > > > Why shouldn't it trigger a write barrier? > > > > > > If the concurrent hit puts a reference to a young generation object into an old generation object it needs to trigger the barrier. So is it not possible for this to happen? Or do we have a delayed trigger? > > > > Delayed. > > What keeps young objects live when they're only in the deferred set? The fact that the GC asks all current compilations to finish before it runs.
Oliver Hunt
Comment 13 2013-08-15 10:46:20 PDT
Comment on attachment 208808 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=208808&action=review I have a problem with initializeLazyWriteBarrier and addLazily. The default and easiest code to right should be the safest, even if it's overly conservative. I can't see anything that makes me think that addConstant can't simply do the initializeLazyWriteBarrier internally. Then if there is a case where it's safe to skip the write barrier and is performance critical we can have unsafeAddConstant or something > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:609 > - m_codeBlock->addConstant(jsNull()); > + initializeLazyWriteBarrier( > + m_codeBlock->addConstantLazily(), > + m_graph.m_plan.writeBarriers, > + m_codeBlock->ownerExecutable(), > + jsNull()); kind of yuck that we're having to put all this code in place of a simple addConstant. Why can't addConstant call initializeLazyWriteBarrier? > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3366 > - byteCodeParser->m_codeBlock->addConstant(JSValue()); > + initializeLazyWriteBarrier( > + byteCodeParser->m_codeBlock->addConstantLazily(), > + byteCodeParser->m_graph.m_plan.writeBarriers, > + byteCodeParser->m_codeBlock->ownerExecutable(), > + JSValue()); Why do we need a write barrier for primitives? If we can't have addConstant to the right work, we should at least have addConstant(enum {UndefinedConstant, NullConstant, EmptyConstant, etc}) or some such
Mark Hahnenberg
Comment 14 2013-08-15 11:47:41 PDT
(In reply to comment #13) > (From update of attachment 208808 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=208808&action=review > > I have a problem with initializeLazyWriteBarrier and addLazily. The default and easiest code to right should be the safest, even if it's overly conservative. I can't see anything that makes me think that addConstant can't simply do the initializeLazyWriteBarrier internally. Then if there is a case where it's safe to skip the write barrier and is performance critical we can have unsafeAddConstant or something > Performance isn't critical. YAGNI. I don't know what you're talking about w.r.t. to conservatism and safety. addConstant would need access to m_graph.m_plan.writeBarriers. That's one reason it can't call initializeLazyWriteBarrier itself. Another reason is that initializeLazyWriteBarrier is something only the DFG cares about, and as such I thought it made less sense to call it from CodeBlock. > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:609 > > - m_codeBlock->addConstant(jsNull()); > > + initializeLazyWriteBarrier( > > + m_codeBlock->addConstantLazily(), > > + m_graph.m_plan.writeBarriers, > > + m_codeBlock->ownerExecutable(), > > + jsNull()); > > kind of yuck that we're having to put all this code in place of a simple addConstant. Why can't addConstant call initializeLazyWriteBarrier? > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3366 > > - byteCodeParser->m_codeBlock->addConstant(JSValue()); > > + initializeLazyWriteBarrier( > > + byteCodeParser->m_codeBlock->addConstantLazily(), > > + byteCodeParser->m_graph.m_plan.writeBarriers, > > + byteCodeParser->m_codeBlock->ownerExecutable(), > > + JSValue()); > > Why do we need a write barrier for primitives? If we can't have addConstant to the right work, we should at least have addConstant(enum {UndefinedConstant, NullConstant, EmptyConstant, etc}) or some such We had a WriteBarrier for primitives before. I chose not to change that in this patch. I was going for consistency in how the ByteCodeParser dealt with constants, but I guess I could just do an addConstant() here and move the WriteBarrier ASSERT back behind the isCell check.
Oliver Hunt
Comment 15 2013-08-15 11:54:51 PDT
(In reply to comment #14) > (In reply to comment #13) > > (From update of attachment 208808 [details] [details]) > > View in context: https://bugs.webkit.org/attachment.cgi?id=208808&action=review > > > > I have a problem with initializeLazyWriteBarrier and addLazily. The default and easiest code to right should be the safest, even if it's overly conservative. I can't see anything that makes me think that addConstant can't simply do the initializeLazyWriteBarrier internally. Then if there is a case where it's safe to skip the write barrier and is performance critical we can have unsafeAddConstant or something > > > Performance isn't critical. YAGNI. > > I don't know what you're talking about w.r.t. to conservatism and safety. > > addConstant would need access to m_graph.m_plan.writeBarriers. That's one reason it can't call initializeLazyWriteBarrier itself. Another reason is that initializeLazyWriteBarrier is something only the DFG cares about, and as such I thought it made less sense to call it from CodeBlock. > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:609 > > > - m_codeBlock->addConstant(jsNull()); > > > + initializeLazyWriteBarrier( > > > + m_codeBlock->addConstantLazily(), > > > + m_graph.m_plan.writeBarriers, > > > + m_codeBlock->ownerExecutable(), > > > + jsNull()); > > > > kind of yuck that we're having to put all this code in place of a simple addConstant. Why can't addConstant call initializeLazyWriteBarrier? > > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3366 > > > - byteCodeParser->m_codeBlock->addConstant(JSValue()); > > > + initializeLazyWriteBarrier( > > > + byteCodeParser->m_codeBlock->addConstantLazily(), > > > + byteCodeParser->m_graph.m_plan.writeBarriers, > > > + byteCodeParser->m_codeBlock->ownerExecutable(), > > > + JSValue()); > > > > Why do we need a write barrier for primitives? If we can't have addConstant to the right work, we should at least have addConstant(enum {UndefinedConstant, NullConstant, EmptyConstant, etc}) or some such > > We had a WriteBarrier for primitives before. I chose not to change that in this patch. I was going for consistency in how the ByteCodeParser dealt with constants, but I guess I could just do an addConstant() here and move the WriteBarrier ASSERT back behind the isCell check. Okay how about DFGBytecodeParse has an addConstant function: void DFGBytecodeParser::addConstant(JSValue value) { initializeLazyWriteBarrier(m_codeBlock->addConstantLazily(), m_graph.m_plan.writeBarriers, m_codeBlock->ownerExecutable(), value) } then m_codeBlock->addConstant(jsNull()) becomes addConstant(jsNull()) and byteCodeParser->m_codeBlock->addConstant(JSValue()); becomes byteCodeParser->addConstant(JSValue());
Mark Hahnenberg
Comment 16 2013-08-15 11:55:32 PDT
(In reply to comment #15) > (In reply to comment #14) > > (In reply to comment #13) > > > (From update of attachment 208808 [details] [details] [details]) > > > View in context: https://bugs.webkit.org/attachment.cgi?id=208808&action=review > > > > > > I have a problem with initializeLazyWriteBarrier and addLazily. The default and easiest code to right should be the safest, even if it's overly conservative. I can't see anything that makes me think that addConstant can't simply do the initializeLazyWriteBarrier internally. Then if there is a case where it's safe to skip the write barrier and is performance critical we can have unsafeAddConstant or something > > > > > Performance isn't critical. YAGNI. > > > > I don't know what you're talking about w.r.t. to conservatism and safety. > > > > addConstant would need access to m_graph.m_plan.writeBarriers. That's one reason it can't call initializeLazyWriteBarrier itself. Another reason is that initializeLazyWriteBarrier is something only the DFG cares about, and as such I thought it made less sense to call it from CodeBlock. > > > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:609 > > > > - m_codeBlock->addConstant(jsNull()); > > > > + initializeLazyWriteBarrier( > > > > + m_codeBlock->addConstantLazily(), > > > > + m_graph.m_plan.writeBarriers, > > > > + m_codeBlock->ownerExecutable(), > > > > + jsNull()); > > > > > > kind of yuck that we're having to put all this code in place of a simple addConstant. Why can't addConstant call initializeLazyWriteBarrier? > > > > > > > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:3366 > > > > - byteCodeParser->m_codeBlock->addConstant(JSValue()); > > > > + initializeLazyWriteBarrier( > > > > + byteCodeParser->m_codeBlock->addConstantLazily(), > > > > + byteCodeParser->m_graph.m_plan.writeBarriers, > > > > + byteCodeParser->m_codeBlock->ownerExecutable(), > > > > + JSValue()); > > > > > > Why do we need a write barrier for primitives? If we can't have addConstant to the right work, we should at least have addConstant(enum {UndefinedConstant, NullConstant, EmptyConstant, etc}) or some such > > > > We had a WriteBarrier for primitives before. I chose not to change that in this patch. I was going for consistency in how the ByteCodeParser dealt with constants, but I guess I could just do an addConstant() here and move the WriteBarrier ASSERT back behind the isCell check. > > Okay how about DFGBytecodeParse has an addConstant function: > > void DFGBytecodeParser::addConstant(JSValue value) > { > initializeLazyWriteBarrier(m_codeBlock->addConstantLazily(), m_graph.m_plan.writeBarriers, m_codeBlock->ownerExecutable(), value) > } > > then > m_codeBlock->addConstant(jsNull()) > > becomes > addConstant(jsNull()) > > and > byteCodeParser->m_codeBlock->addConstant(JSValue()); > > becomes > byteCodeParser->addConstant(JSValue()); I like this :-)
Mark Hahnenberg
Comment 17 2013-08-15 14:02:30 PDT
WebKit Commit Bot
Comment 18 2013-08-15 20:27:30 PDT
Comment on attachment 208852 [details] Patch Clearing flags on attachment: 208852 Committed r154162: <http://trac.webkit.org/changeset/154162>
WebKit Commit Bot
Comment 19 2013-08-15 20:27:33 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.