[webkit-reviews] review granted: [Bug 177844] Add support for using Probe DFG OSR Exit behind a runtime flag. : [Attachment 322619] proposed patch.

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Mark Lam <mark.lam at apple.com>'s
request for review:
Bug 177844: Add support for using Probe DFG OSR Exit behind a runtime flag.
https://bugs.webkit.org/show_bug.cgi?id=177844

Attachment 322619: proposed patch.

https://bugs.webkit.org/attachment.cgi?id=322619&action=review




--- Comment #3 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 322619
  --> https://bugs.webkit.org/attachment.cgi?id=322619
proposed patch.

View in context: https://bugs.webkit.org/attachment.cgi?id=322619&action=review

> Source/JavaScriptCore/assembler/ProbeStack.cpp:99
> +    RELEASE_ASSERT(!dirtyBits);

don't think this is needed since the above loop condition is exactly this.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:50
>
+//============================================================================
===========

I don't think this is needed to delineate.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:444
> +    // The only reason for using this do while look is so we can break out
midway when appropriate.

Nit: I think we've started to use immediately invoked lambdas in this
situation, but I'm also ok with the while loop.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:487
> +	   if (extraInitializationLevel <=
ExtraInitializationLevel::SpeculationRecovery)

I'm confused about this extraInitialzationLevel stuff. Is this something we
cache on first exit? Does it actually help with speed? This all seems like
straight forward code, so I can't imagine it would help, you don't loop
anywhere here.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:557
> +    size_t nextUndefinedSpanIndex = 0;
> +    size_t nextUndefinedOperandIndex = numberOfOperands;
> +    if (numUndefinedOperandSpans)
> +	   nextUndefinedOperandIndex =
undefinedOperandSpans[nextUndefinedSpanIndex].firstIndex;
> +
> +    JSValue undefined = jsUndefined();
> +    for (size_t spanIndex = 0; spanIndex < numUndefinedOperandSpans;
++spanIndex) {
> +	   auto& span = undefinedOperandSpans[spanIndex];
> +	   int firstOffset = span.minOffset;
> +	   int lastOffset = firstOffset + span.numberOfRegisters;
> +
> +	   for (int offset = firstOffset; offset < lastOffset; ++offset)
> +	       frame.setOperand(offset, undefined);
> +    }

I thought you said this optimization doesn't help? If it doesn't, I saw we drop
it, since it complicates the code.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:671
> +    // The tag registers are needed to materialize recoveries below.

This comment seems like it has a high chance of being wrong since this is C++
code.

> Source/JavaScriptCore/dfg/DFGOSRExit.cpp:903
>
+//============================================================================
===========

I don't think this is needed to delineate.