Bug 119833 – Concurrent compilation thread should not trigger WriteBarriers

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

[Mark Hahnenberg]

The concurrent compilation should interact minimally with the Heap, including triggering WriteBarriers. This is a prerequisite for generational GC.

[Mark Hahnenberg]

The concurrent compilation thread*

[Mark Hahnenberg]

Created attachment 208789 [details]
Patch

[Early Warning System Bot]

Comment on attachment 208789 [details]
Patch

Attachment 208789 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1469082

[Early Warning System Bot]

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

[Mark Hahnenberg]

Created attachment 208790 [details]
Patch

[EFL EWS Bot]

Comment on attachment 208790 [details]
Patch

Attachment 208790 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1461606

[EFL EWS Bot]

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

[Mark Hahnenberg]

Created attachment 208808 [details]
Patch

[Oliver Hunt]

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]

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

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

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

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

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

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

Created attachment 208852 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 208852 [details]
Patch

Clearing flags on attachment: 208852

Committed r154162: <http://trac.webkit.org/changeset/154162>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.