Attachment 340254 Details for Bug 185583 – the patch

[patch] the patch

blah.patch (text/plain), 15.88 KB, created by Filip Pizlo on 2018-05-12 15:27:51 PDT

(hide)

Description:

Filename:

MIME Type:

Creator: Filip Pizlo

Created: 2018-05-12 15:27:51 PDT

Size: 15.88 KB

patch

obsolete

>Index: Source/JavaScriptCore/ChangeLog
>===================================================================
>--- Source/JavaScriptCore/ChangeLog	(revision 231735)
>+++ Source/JavaScriptCore/ChangeLog	(working copy)
>@@ -1,3 +1,40 @@
>+2018-05-12  Filip Pizlo  <fpizlo@apple.com>
>+
>+        CachedCall::call() should be faster
>+        https://bugs.webkit.org/show_bug.cgi?id=185583
>+
>+        Reviewed by NOBODY (OOPS!).
>+        
>+        CachedCall is an optimization for String.prototype.replace(r, f) where f is a function.
>+        Unfortunately, because of a combination of abstraction and assertions, this code path had a
>+        lot of overhead. This patch reduces this overhead by:
>+        
>+        - Turning off some assertions. These assertions don't look to have security value; they're
>+          mostly for sanity. I turned off stack alignment checks and VM state checks having to do
>+          with whether the JSLock is held. The JSLock checks are not relevant when doing a cached
>+          call, considering that the caller would have already been strongly assuming that the JSLock
>+          is held.
>+        
>+        - Making more things inlineable.
>+        
>+        This looks like a small (4% ish) speed-up on SunSpider/string-unpack-code.
>+
>+        * JavaScriptCore.xcodeproj/project.pbxproj:
>+        * interpreter/CachedCall.h:
>+        (JSC::CachedCall::call):
>+        * interpreter/Interpreter.cpp:
>+        (JSC::checkedReturn): Deleted.
>+        * interpreter/Interpreter.h:
>+        (JSC::Interpreter::checkedReturn):
>+        * interpreter/InterpreterInlines.h:
>+        (JSC::Interpreter::execute):
>+        * jit/JITCode.cpp:
>+        (JSC::JITCode::execute): Deleted.
>+        * jit/JITCodeInlines.h: Added.
>+        (JSC::JITCode::execute):
>+        * llint/LowLevelInterpreter.asm:
>+        * runtime/StringPrototype.cpp:
>+
> 2018-05-11  Caio Lima  <ticaiolima@gmail.com>
> 
>         [ESNext][BigInt] Implement support for "*" operation
>Index: Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj
>===================================================================
>--- Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj	(revision 231686)
>+++ Source/JavaScriptCore/JavaScriptCore.xcodeproj/project.pbxproj	(working copy)
>@@ -712,6 +712,7 @@
> 		0FF9CE741B9CD6D0004EDCA6 /* PolymorphicAccess.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FF9CE721B9CD6D0004EDCA6 /* PolymorphicAccess.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 		0FFA549816B8835300B3A982 /* A64DOpcode.h in Headers */ = {isa = PBXBuildFile; fileRef = 652A3A231651C69700A80AFE /* A64DOpcode.h */; settings = {ATTRIBUTES = (Private, ); }; };
> 		0FFB6C391AF48DDC00DB1BF7 /* TypeofType.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FFB6C371AF48DDC00DB1BF7 /* TypeofType.h */; settings = {ATTRIBUTES = (Private, ); }; };
>+		0FFB80BC20A794730006AAF6 /* JITCodeInlines.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FFB80BB20A794700006AAF6 /* JITCodeInlines.h */; };
> 		0FFB921A16D02EC50055A5DB /* DFGBasicBlockInlines.h in Headers */ = {isa = PBXBuildFile; fileRef = 0FD5652216AB780A00197653 /* DFGBasicBlockInlines.h */; };
> 		0FFB921C16D02F110055A5DB /* DFGOSRExitCompilationInfo.h in Headers */ = {isa = PBXBuildFile; fileRef = 65987F2C167FE84B003C2F8D /* DFGOSRExitCompilationInfo.h */; };
> 		0FFB921D16D02F300055A5DB /* DFGSlowPathGenerator.h in Headers */ = {isa = PBXBuildFile; fileRef = 0F1E3A501537C2CB000F9456 /* DFGSlowPathGenerator.h */; };
>@@ -2990,6 +2991,7 @@
> 		0FF9CE721B9CD6D0004EDCA6 /* PolymorphicAccess.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = PolymorphicAccess.h; sourceTree = "<group>"; };
> 		0FFB6C361AF48DDC00DB1BF7 /* TypeofType.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = TypeofType.cpp; sourceTree = "<group>"; };
> 		0FFB6C371AF48DDC00DB1BF7 /* TypeofType.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = TypeofType.h; sourceTree = "<group>"; };
>+		0FFB80BB20A794700006AAF6 /* JITCodeInlines.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = JITCodeInlines.h; sourceTree = "<group>"; };
> 		0FFC920F1B94D4DF0071DD66 /* InferredTypeTable.cpp */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.cpp.cpp; path = InferredTypeTable.cpp; sourceTree = "<group>"; };
> 		0FFC92101B94D4DF0071DD66 /* InferredTypeTable.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; path = InferredTypeTable.h; sourceTree = "<group>"; };
> 		0FFC92131B94E83E0071DD66 /* DFGDesiredInferredType.h */ = {isa = PBXFileReference; fileEncoding = 4; lastKnownFileType = sourcecode.c.h; name = DFGDesiredInferredType.h; path = dfg/DFGDesiredInferredType.h; sourceTree = "<group>"; };
>@@ -5514,6 +5516,7 @@
> 				146FE51111A710430087AE66 /* JITCall32_64.cpp */,
> 				0F8F94431667635200D61971 /* JITCode.cpp */,
> 				86CCEFDD0F413F8900FD7F9E /* JITCode.h */,
>+				0FFB80BB20A794700006AAF6 /* JITCodeInlines.h */,
> 				FE476FF3207E85D40093CA2D /* JITCodeMap.h */,
> 				0F0776BD14FF002800102332 /* JITCompilationEffort.h */,
> 				0FAF7EFA165BA919000C8455 /* JITDisassembler.cpp */,
>@@ -9243,6 +9246,7 @@
> 				E34E657520668EAA00FB81AC /* ParseHash.h in Headers */,
> 				37C738D21EDB56E4003F2B0B /* ParseInt.h in Headers */,
> 				BC18C44B0E16F5CD00B34460 /* Parser.h in Headers */,
>+				0FFB80BC20A794730006AAF6 /* JITCodeInlines.h in Headers */,
> 				93052C350FB792190048FDC3 /* ParserArena.h in Headers */,
> 				0FCCAE4516D0CF7400D0C65B /* ParserError.h in Headers */,
> 				A77F1825164192C700640A47 /* ParserModes.h in Headers */,
>Index: Source/JavaScriptCore/interpreter/CachedCall.h
>===================================================================
>--- Source/JavaScriptCore/interpreter/CachedCall.h	(revision 231686)
>+++ Source/JavaScriptCore/interpreter/CachedCall.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2009-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2009-2018 Apple Inc. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>  * modification, are permitted provided that the following conditions
>@@ -60,7 +60,7 @@ namespace JSC {
>             m_valid = !scope.exception();
>         }
>         
>-        JSValue call()
>+        ALWAYS_INLINE JSValue call()
>         { 
>             ASSERT(m_valid);
>             ASSERT(m_arguments.size() == static_cast<size_t>(m_protoCallFrame.argumentCount()));
>Index: Source/JavaScriptCore/interpreter/Interpreter.cpp
>===================================================================
>--- Source/JavaScriptCore/interpreter/Interpreter.cpp	(revision 231686)
>+++ Source/JavaScriptCore/interpreter/Interpreter.cpp	(working copy)
>@@ -47,6 +47,7 @@
> #include "FrameTracers.h"
> #include "FunctionCodeBlock.h"
> #include "InterpreterInlines.h"
>+#include "JITCodeInlines.h"
> #include "JSArrayInlines.h"
> #include "JSBoundFunction.h"
> #include "JSCInlines.h"
>@@ -769,18 +770,6 @@ void Interpreter::notifyDebuggerOfExcept
>     exception->setDidNotifyInspectorOfThrow();
> }
> 
>-static inline JSValue checkedReturn(JSValue returnValue)
>-{
>-    ASSERT(returnValue);
>-    return returnValue;
>-}
>-
>-static inline JSObject* checkedReturn(JSObject* returnValue)
>-{
>-    ASSERT(returnValue);
>-    return returnValue;
>-}
>-
> JSValue Interpreter::executeProgram(const SourceCode& source, CallFrame* callFrame, JSObject* thisObj)
> {
>     JSScope* scope = thisObj->globalObject()->globalScope();
>@@ -1128,31 +1117,6 @@ CallFrameClosure Interpreter::prepareFor
>     return result;
> }
> 
>-JSValue Interpreter::execute(CallFrameClosure& closure) 
>-{
>-    VM& vm = *closure.vm;
>-    auto throwScope = DECLARE_THROW_SCOPE(vm);
>-
>-    ASSERT(!vm.isCollectorBusyOnCurrentThread());
>-    RELEASE_ASSERT(vm.currentThreadIsHoldingAPILock());
>-    if (vm.isCollectorBusyOnCurrentThread())
>-        return jsNull();
>-
>-    StackStats::CheckPoint stackCheckPoint;
>-
>-    VMTraps::Mask mask(VMTraps::NeedTermination, VMTraps::NeedWatchdogCheck);
>-    if (UNLIKELY(vm.needTrapHandling(mask))) {
>-        vm.handleTraps(closure.oldCallFrame, mask);
>-        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
>-    }
>-
>-    // Execute the code:
>-    throwScope.release();
>-    JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);
>-
>-    return checkedReturn(result);
>-}
>-
> JSValue Interpreter::execute(EvalExecutable* eval, CallFrame* callFrame, JSValue thisValue, JSScope* scope)
> {
>     VM& vm = *scope->vm();
>Index: Source/JavaScriptCore/interpreter/Interpreter.h
>===================================================================
>--- Source/JavaScriptCore/interpreter/Interpreter.h	(revision 231686)
>+++ Source/JavaScriptCore/interpreter/Interpreter.h	(working copy)
>@@ -1,5 +1,5 @@
> /*
>- * Copyright (C) 2008-2017 Apple Inc. All rights reserved.
>+ * Copyright (C) 2008-2018 Apple Inc. All rights reserved.
>  * Copyright (C) 2012 Research In Motion Limited. All rights reserved.
>  *
>  * Redistribution and use in source and binary forms, with or without
>@@ -133,6 +133,18 @@ namespace JSC {
> 
>     private:
>         enum ExecutionFlag { Normal, InitializeAndReturn };
>+        
>+        static JSValue checkedReturn(JSValue returnValue)
>+        {
>+            ASSERT(returnValue);
>+            return returnValue;
>+        }
>+        
>+        static JSObject* checkedReturn(JSObject* returnValue)
>+        {
>+            ASSERT(returnValue);
>+            return returnValue;
>+        }
> 
>         CallFrameClosure prepareForRepeatCall(FunctionExecutable*, CallFrame*, ProtoCallFrame*, JSFunction*, int argumentCountIncludingThis, JSScope*, const ArgList&);
> 
>Index: Source/JavaScriptCore/interpreter/InterpreterInlines.h
>===================================================================
>--- Source/JavaScriptCore/interpreter/InterpreterInlines.h	(revision 231686)
>+++ Source/JavaScriptCore/interpreter/InterpreterInlines.h	(working copy)
>@@ -26,6 +26,8 @@
> 
> #pragma once
> 
>+#include "CallFrameClosure.h"
>+#include "Exception.h"
> #include "Instruction.h"
> #include "Interpreter.h"
> #include "JSCPtrTag.h"
>@@ -71,4 +73,27 @@ inline OpcodeID Interpreter::getOpcodeID
>     return instruction.u.opcode;
> }
> 
>+ALWAYS_INLINE JSValue Interpreter::execute(CallFrameClosure& closure)
>+{
>+    VM& vm = *closure.vm;
>+    auto throwScope = DECLARE_THROW_SCOPE(vm);
>+
>+    ASSERT(!vm.isCollectorBusyOnCurrentThread());
>+    ASSERT(vm.currentThreadIsHoldingAPILock());
>+
>+    StackStats::CheckPoint stackCheckPoint;
>+
>+    VMTraps::Mask mask(VMTraps::NeedTermination, VMTraps::NeedWatchdogCheck);
>+    if (UNLIKELY(vm.needTrapHandling(mask))) {
>+        vm.handleTraps(closure.oldCallFrame, mask);
>+        RETURN_IF_EXCEPTION(throwScope, throwScope.exception());
>+    }
>+
>+    // Execute the code:
>+    throwScope.release();
>+    JSValue result = closure.functionExecutable->generatedJITCodeForCall()->execute(&vm, closure.protoCallFrame);
>+
>+    return checkedReturn(result);
>+}
>+
> } // namespace JSC
>Index: Source/JavaScriptCore/jit/JITCode.cpp
>===================================================================
>--- Source/JavaScriptCore/jit/JITCode.cpp	(revision 231686)
>+++ Source/JavaScriptCore/jit/JITCode.cpp	(working copy)
>@@ -26,7 +26,6 @@
> #include "config.h"
> #include "JITCode.h"
> 
>-#include "LLIntThunks.h"
> #include "JSCInlines.h"
> #include "ProtoCallFrame.h"
> #include <wtf/PrintStream.h>
>@@ -67,15 +66,6 @@ void JITCode::validateReferences(const T
> {
> }
> 
>-JSValue JITCode::execute(VM* vm, ProtoCallFrame* protoCallFrame)
>-{
>-    auto scope = DECLARE_THROW_SCOPE(*vm);
>-    void* entryAddress;
>-    entryAddress = addressForCall(MustCheckArity).executableAddress();
>-    JSValue result = JSValue::decode(vmEntryToJavaScript(entryAddress, vm, protoCallFrame));
>-    return scope.exception() ? jsNull() : result;
>-}
>-
> DFG::CommonData* JITCode::dfgCommon()
> {
>     RELEASE_ASSERT_NOT_REACHED();
>Index: Source/JavaScriptCore/jit/JITCodeInlines.h
>===================================================================
>--- Source/JavaScriptCore/jit/JITCodeInlines.h	(nonexistent)
>+++ Source/JavaScriptCore/jit/JITCodeInlines.h	(working copy)
>@@ -0,0 +1,43 @@
>+/*
>+ * Copyright (C) 2018 Apple Inc. All rights reserved.
>+ *
>+ * Redistribution and use in source and binary forms, with or without
>+ * modification, are permitted provided that the following conditions
>+ * are met:
>+ * 1. Redistributions of source code must retain the above copyright
>+ *    notice, this list of conditions and the following disclaimer.
>+ * 2. Redistributions in binary form must reproduce the above copyright
>+ *    notice, this list of conditions and the following disclaimer in the
>+ *    documentation and/or other materials provided with the distribution.
>+ *
>+ * THIS SOFTWARE IS PROVIDED BY APPLE INC. ``AS IS'' AND ANY
>+ * EXPRESS OR IMPLIED WARRANTIES, INCLUDING, BUT NOT LIMITED TO, THE
>+ * IMPLIED WARRANTIES OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR
>+ * PURPOSE ARE DISCLAIMED.  IN NO EVENT SHALL APPLE INC. OR
>+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL, SPECIAL,
>+ * EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT NOT LIMITED TO,
>+ * PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES; LOSS OF USE, DATA, OR
>+ * PROFITS; OR BUSINESS INTERRUPTION) HOWEVER CAUSED AND ON ANY THEORY
>+ * OF LIABILITY, WHETHER IN CONTRACT, STRICT LIABILITY, OR TORT
>+ * (INCLUDING NEGLIGENCE OR OTHERWISE) ARISING IN ANY WAY OUT OF THE USE
>+ * OF THIS SOFTWARE, EVEN IF ADVISED OF THE POSSIBILITY OF SUCH DAMAGE. 
>+ */
>+
>+#pragma once
>+
>+#include "JITCode.h"
>+#include "LLIntThunks.h"
>+
>+namespace JSC {
>+
>+ALWAYS_INLINE JSValue JITCode::execute(VM* vm, ProtoCallFrame* protoCallFrame)
>+{
>+    auto scope = DECLARE_THROW_SCOPE(*vm);
>+    void* entryAddress;
>+    entryAddress = addressForCall(MustCheckArity).executableAddress();
>+    JSValue result = JSValue::decode(vmEntryToJavaScript(entryAddress, vm, protoCallFrame));
>+    return scope.exception() ? jsNull() : result;
>+}
>+
>+} // namespave JSC
>+
>Index: Source/JavaScriptCore/llint/LowLevelInterpreter.asm
>===================================================================
>--- Source/JavaScriptCore/llint/LowLevelInterpreter.asm	(revision 231686)
>+++ Source/JavaScriptCore/llint/LowLevelInterpreter.asm	(working copy)
>@@ -542,22 +542,24 @@ else
> end
> 
> macro checkStackPointerAlignment(tempReg, location)
>-    if ARM64 or ARM64E or C_LOOP
>-        # ARM64 and ARM64E will check for us!
>-        # C_LOOP does not need the alignment, and can use a little perf
>-        # improvement from avoiding useless work.
>-    else
>-        if ARM or ARMv7 or ARMv7_TRADITIONAL
>-            # ARM can't do logical ops with the sp as a source
>-            move sp, tempReg
>-            andp StackAlignmentMask, tempReg
>+    if ASSERT_ENABLED
>+        if ARM64 or ARM64E or C_LOOP
>+            # ARM64 and ARM64E will check for us!
>+            # C_LOOP does not need the alignment, and can use a little perf
>+            # improvement from avoiding useless work.
>         else
>-            andp sp, StackAlignmentMask, tempReg
>+            if ARM or ARMv7 or ARMv7_TRADITIONAL
>+                # ARM can't do logical ops with the sp as a source
>+                move sp, tempReg
>+                andp StackAlignmentMask, tempReg
>+            else
>+                andp sp, StackAlignmentMask, tempReg
>+            end
>+            btpz tempReg, .stackPointerOkay
>+            move location, tempReg
>+            break
>+        .stackPointerOkay:
>         end
>-        btpz tempReg, .stackPointerOkay
>-        move location, tempReg
>-        break
>-    .stackPointerOkay:
>     end
> end
> 
>Index: Source/JavaScriptCore/runtime/StringPrototype.cpp
>===================================================================
>--- Source/JavaScriptCore/runtime/StringPrototype.cpp	(revision 231686)
>+++ Source/JavaScriptCore/runtime/StringPrototype.cpp	(working copy)
>@@ -28,7 +28,9 @@
> #include "CachedCall.h"
> #include "Error.h"
> #include "FrameTracers.h"
>+#include "InterpreterInlines.h"
> #include "IntlObject.h"
>+#include "JITCodeInlines.h"
> #include "JSArray.h"
> #include "JSCBuiltins.h"
> #include "JSCInlines.h"

Flags:

ysuzuki: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 185583: 340254