Bug 119833 - Concurrent compilation thread should not trigger WriteBarriers
Summary: Concurrent compilation thread should not trigger WriteBarriers
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Hahnenberg
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2013-08-14 19:32 PDT by Mark Hahnenberg
Modified: 2013-08-15 20:27 PDT (History)
6 users (show)

See Also:


Attachments
Patch (44.99 KB, patch)
2013-08-14 21:25 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (47.18 KB, patch)
2013-08-14 21:38 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (47.19 KB, patch)
2013-08-15 07:42 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff
Patch (46.29 KB, patch)
2013-08-15 14:02 PDT, Mark Hahnenberg
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Hahnenberg 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.
Comment 1 Mark Hahnenberg 2013-08-14 19:33:23 PDT
The concurrent compilation thread*
Comment 2 Mark Hahnenberg 2013-08-14 21:25:10 PDT
Created attachment 208789 [details]
Patch
Comment 3 Early Warning System Bot 2013-08-14 21:35:09 PDT
Comment on attachment 208789 [details]
Patch

Attachment 208789 [details] did not pass qt-ews (qt):
Output: http://webkit-queues.appspot.com/results/1469082
Comment 4 Early Warning System Bot 2013-08-14 21:36:26 PDT
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
Comment 5 Mark Hahnenberg 2013-08-14 21:38:19 PDT
Created attachment 208790 [details]
Patch
Comment 6 EFL EWS Bot 2013-08-14 22:13:02 PDT
Comment on attachment 208790 [details]
Patch

Attachment 208790 [details] did not pass efl-ews (efl):
Output: http://webkit-queues.appspot.com/results/1461606
Comment 7 EFL EWS Bot 2013-08-14 22:28:12 PDT
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
Comment 8 Mark Hahnenberg 2013-08-15 07:42:02 PDT
Created attachment 208808 [details]
Patch
Comment 9 Oliver Hunt 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?
Comment 10 Mark Hahnenberg 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.
Comment 11 Oliver Hunt 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?
Comment 12 Mark Hahnenberg 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.
Comment 13 Oliver Hunt 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
Comment 14 Mark Hahnenberg 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.
Comment 15 Oliver Hunt 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());
Comment 16 Mark Hahnenberg 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 :-)
Comment 17 Mark Hahnenberg 2013-08-15 14:02:30 PDT
Created attachment 208852 [details]
Patch
Comment 18 WebKit Commit Bot 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>
Comment 19 WebKit Commit Bot 2013-08-15 20:27:33 PDT
All reviewed patches have been landed.  Closing bug.