[webkit-reviews] review granted: [Bug 181535] [DFG][FTL] Introduce PhantomNewRegexp and RegExpExecNonGlobalOrSticky : [Attachment 331470] Patch

[bugzilla-daemon at webkit.org]

Saam Barati <sbarati at apple.com> has granted Yusuke Suzuki
<utatane.tea at gmail.com>'s request for review:
Bug 181535: [DFG][FTL] Introduce PhantomNewRegexp and
RegExpExecNonGlobalOrSticky
https://bugs.webkit.org/show_bug.cgi?id=181535

Attachment 331470: Patch

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




--- Comment #18 from Saam Barati <sbarati at apple.com> ---
Comment on attachment 331470
  --> https://bugs.webkit.org/attachment.cgi?id=331470
Patch

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

r=me

> Source/JavaScriptCore/ChangeLog:13
> +	   phase. We should do this analysis in OAS phase since we would like
to extend it further by tracking `lastIndex` modification

"we would like to extend it further by tracking ..." => "we track modifications
to `lastIndex` in the OAS phase."

> Source/JavaScriptCore/ChangeLog:15
> +	   if we carefully model SetRegExpObjectLastIndex and
GetRegExpObjectLastIndex in OAS phase.

"if we" => "because we"

> Source/JavaScriptCore/ChangeLog:19
> +	   non-global and non-sticky one. We can later extend this optimization
for RegExp with global flag. But this is not included
> +	   in this patch.

Is there a bug open for this? Is it worth doing?

> Source/JavaScriptCore/ChangeLog:37
> +	   This patch does not change Octane/RegExp so much since it heavily
uses String.prototype.replace, which is not handled in
> +	   this patch right now. We should support StringReplace node in
subsequent patches.

Any idea if this improves Web Tooling Bench? I know many of its tests heavily
use RegExps.

> Source/JavaScriptCore/dfg/DFGOperations.cpp:1045
> +	   scope.release();

Why scope.release() here? The below line isn't going to throw? Maybe:
ASSERT(!scope.exception())

> Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp:10628
> +    GPRReg argumentGPR = argument.gpr();

This is inconsistent with what you do in the FTL. The FTL does lowString(). My
vote would be for this node to have a StringUse for its second operand instead
of KnownCellUse. AI will already prove this check isn't needed.

> JSTests/stress/materialize-regexp-at-osr-exit.js:1
> +function shouldBe(actual, expected)

You should add some tests where lastIndex and another object allocation form a
cycle in various ways. And make sure things work if it gets escaped, does not
get escaped, etc.