[webkit-reviews] review denied: [Bug 119833] Concurrent compilation thread should not trigger WriteBarriers : [Attachment 208808] Patch

[bugzilla-daemon at webkit.org]

Oliver Hunt <oliver at apple.com> has denied Mark Hahnenberg
<mhahnenberg at apple.com>'s request for review:
Bug 119833: Concurrent compilation thread should not trigger WriteBarriers
https://bugs.webkit.org/show_bug.cgi?id=119833

Attachment 208808: Patch
https://bugs.webkit.org/attachment.cgi?id=208808&action=review

------- Additional Comments from Oliver Hunt <oliver at apple.com>
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