Attachment 361225 Details for Bug 194307 – proposed patch.

[patch] proposed patch.

bug-194307.patch (text/plain), 5.89 KB, created by Mark Lam on 2019-02-05 15:14:13 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Mark Lam

Created: 2019-02-05 15:14:13 PST

Size: 5.89 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 240995)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,45 @@
>+2019-02-05  Mark Lam  <mark.lam@apple.com>
>+
>+        Fix DFG's doesGC() for a few more nodes.
>+        https://bugs.webkit.org/show_bug.cgi?id=194307
>+        <rdar://problem/47832956>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Fix doesGC() for the following nodes:
>+
>+            NumberToStringWithValidRadixConstant:
>+                Calls operationInt32ToStringWithValidRadix(), which calls int32ToString(),
>+                which can allocate a string.
>+                Calls operationInt52ToStringWithValidRadix(), which calls int52ToString(),
>+                which can allocate a string.
>+                Calls operationDoubleToStringWithValidRadix(), which calls numberToString(),
>+                which can allocate a string.
>+
>+            RegExpExecNonGlobalOrSticky: calls createRegExpMatchesArray() which allocates
>+                memory for all kinds of objects.
>+            RegExpMatchFast: calls operationRegExpMatchFastString(), which calls
>+                RegExpObject::execInline() and RegExpObject::matchGlobal().  Both of
>+                these allocates memory for the match result.
>+            RegExpMatchFastGlobal: calls operationRegExpMatchFastGlobalString(), which
>+                calls RegExpObject's collectMatches(), which allocates an array amongst
>+                other objects.
>+
>+            StringFromCharCode:
>+                If the uint32 code to convert is greater than maxSingleCharacterString,
>+                we'll call operationStringFromCharCode(), which calls jsSingleCharacterString(),
>+                which allocates a new string if the code is greater than maxSingleCharacterString.
>+
>+        Also fix SpeculativeJIT::compileFromCharCode() and FTL's compileStringFromCharCode()
>+        to use maxSingleCharacterString instead of a literal constant.
>+
>+        * dfg/DFGDoesGC.cpp:
>+        (JSC::DFG::doesGC):
>+        * dfg/DFGSpeculativeJIT.cpp:
>+        (JSC::DFG::SpeculativeJIT::compileFromCharCode):
>+        * ftl/FTLLowerDFGToB3.cpp:
>+        (JSC::FTL::DFG::LowerDFGToB3::compileStringFromCharCode):
>+
> 2019-02-05  Keith Rollin  <krollin@apple.com>
> 
>         Enable the automatic checking and regenerations of .xcfilelists during builds
>Index: Source/JavaScriptCore/dfg/DFGDoesGC.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGDoesGC.cpp	(revision 240991)
>+++ Source/JavaScriptCore/dfg/DFGDoesGC.cpp	(working copy)
>@@ -121,9 +121,6 @@ bool doesGC(Graph& graph, Node* node)
>     case CheckNotEmpty:
>     case AssertNotEmpty:
>     case CheckStringIdent:
>-    case RegExpExecNonGlobalOrSticky:
>-    case RegExpMatchFast:
>-    case RegExpMatchFastGlobal:
>     case CompareLess:
>     case CompareLessEq:
>     case CompareGreater:
>@@ -150,7 +147,6 @@ bool doesGC(Graph& graph, Node* node)
>     case IsTypedArrayView:
>     case TypeOf:
>     case LogicalNot:
>-    case NumberToStringWithValidRadixConstant:
>     case Jump:
>     case Branch:
>     case Switch:
>@@ -165,7 +161,6 @@ bool doesGC(Graph& graph, Node* node)
>     case ForceOSRExit:
>     case CPUIntrinsic:
>     case CheckTraps:
>-    case StringFromCharCode:
>     case NormalizeMapKey:
>     case GetMapBucket:
>     case GetMapBucketHead:
>@@ -301,6 +296,7 @@ bool doesGC(Graph& graph, Node* node)
>     case InstanceOfCustom:
>     case LoadVarargs:
>     case NumberToStringWithRadix:
>+    case NumberToStringWithValidRadixConstant:
>     case PutById:
>     case PutByIdDirect:
>     case PutByIdFlush:
>@@ -316,6 +312,9 @@ bool doesGC(Graph& graph, Node* node)
>     case PutStack:
>     case PutToArguments:
>     case RegExpExec:
>+    case RegExpExecNonGlobalOrSticky:
>+    case RegExpMatchFast:
>+    case RegExpMatchFastGlobal:
>     case RegExpTest:
>     case ResolveScope:
>     case ResolveScopeForHoistingFuncDeclInEval:
>@@ -417,6 +416,13 @@ bool doesGC(Graph& graph, Node* node)
>             return false;
>         return true;
> 
>+    case StringFromCharCode:
>+        // FIXME: Should we constant fold this case?
>+        // https://bugs.webkit.org/show_bug.cgi?id=194308
>+        if (node->child1()->isInt32Constant() && (node->child1()->asUInt32() <= maxSingleCharacterString))
>+            return false;
>+        return true;
>+
>     case LastNodeType:
>         RELEASE_ASSERT_NOT_REACHED();
>         return true;
>Index: Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp
>===================================================================
>--- Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(revision 240991)
>+++ Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp	(working copy)
>@@ -2282,7 +2282,7 @@ void SpeculativeJIT::compileFromCharCode
>     GPRReg smallStringsReg = smallStrings.gpr();
> 
>     JITCompiler::JumpList slowCases;
>-    slowCases.append(m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, TrustedImm32(0xff)));
>+    slowCases.append(m_jit.branch32(MacroAssembler::AboveOrEqual, propertyReg, TrustedImm32(maxSingleCharacterString)));
>     m_jit.move(TrustedImmPtr(m_jit.vm()->smallStrings.singleCharacterStrings()), smallStringsReg);
>     m_jit.loadPtr(MacroAssembler::BaseIndex(smallStringsReg, propertyReg, MacroAssembler::ScalePtr, 0), scratchReg);
> 
>Index: Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp
>===================================================================
>--- Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(revision 240991)
>+++ Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp	(working copy)
>@@ -6708,7 +6708,7 @@ private:
>         LBasicBlock continuation = m_out.newBlock();
> 
>         m_out.branch(
>-            m_out.aboveOrEqual(value, m_out.constInt32(0xff)),
>+            m_out.aboveOrEqual(value, m_out.constInt32(maxSingleCharacterString)),
>             rarely(slowCase), usually(smallIntCase));
> 
>         LBasicBlock lastNext = m_out.appendTo(smallIntCase, slowCase);

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194307: 361225