The concurrent compilation should interact minimally with the Heap, including triggering WriteBarriers. This is a prerequisite for generational GC.
The concurrent compilation thread*
Created attachment 208789 [details] Patch
Comment on attachment 208789 [details] Patch Attachment 208789 [details] did not pass qt-ews (qt): Output: http://webkit-queues.appspot.com/results/1469082
Comment on attachment 208789 [details] Patch Attachment 208789 [details] did not pass qt-wk2-ews (qt-wk2): Output: http://webkit-queues.appspot.com/results/1462338
Created attachment 208790 [details] Patch
Comment on attachment 208790 [details] Patch Attachment 208790 [details] did not pass efl-ews (efl): Output: http://webkit-queues.appspot.com/results/1461606
Comment on attachment 208790 [details] Patch Attachment 208790 [details] did not pass efl-wk2-ews (efl-wk2): Output: http://webkit-queues.appspot.com/results/1466178
Created attachment 208808 [details] Patch
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?
(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.
(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?
(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.
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
(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.
(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());
(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 :-)
Created attachment 208852 [details] Patch
Comment on attachment 208852 [details] Patch Clearing flags on attachment: 208852 Committed r154162: <http://trac.webkit.org/changeset/154162>
All reviewed patches have been landed. Closing bug.