Skip to content

Commit 18d9fef

Browse files
author
Alexey Shvayka
committed
[JSC] Skip ProxyObject's ICs trap result validation in the common case
https://bugs.webkit.org/show_bug.cgi?id=255661 <rdar://problem/108269769> Reviewed by Yusuke Suzuki. Invariants of the essential internal methods [1] are enforced for all objects, both ordinary and exotic, to avoid them having unpredictable behavior that negatively impacts developer experience and creates security issues. Enforcing them for Proxy objects is especially cumbersome, given traps are userland code, and quite expensive performance-wise: invariants invalidation requires checking property attributes, which currently requires JS => C++ call. After carefully analyzing all the invariants of the all internal methods, it was concluded that if [[ProxyTarget]] is an ordinary extensible object without non-configurable properties, validation of the invariants can be safely (non-observably) skipped. trap name IC attributes checked ======================== == ================================== getOwnPropertyDescriptor no non-configurable || non-extensible defineProperty no non-configurable || non-extensible has yes non-configurable || non-extensible get yes non-configurable && (non-writable || accessor) set yes non-configurable && (non-writable || accessor) deleteProperty maybe non-configurable || non-extensible ownKeys no non-configurable || non-extensible This change: 1. Introduces OverridesIsExtensible out-of-line type info flag to tighten Structure's didPreventExtensions() flag, making it more pessimistic: it being `true` doesn't necessarily mean the structure is non-extensible, but the reverse of it being `false` can be safely relied on. It's not a completely new approach: #11843 does the same thing. This change was proved not to introduce bugs since Structure's didPreventExtensions() flag is currently checked only for objects without isExtensible() override. 2. Introduces hasNonConfigurableProperties and hasNonConfigurableReadOnlyOrGetterSetterProperties Structure flags that are also pessimistic. When inferring their initial values, we do extra care not to be overly pessimistic for objects except JSFinalObject by checking overridesGetOwnPropertySlot() so that userland code, which heavily uses Map / Set as [[ProxyTarget]], more performant, while making an exception for JSArray's non-configurable non-structure "length" property. While there is existing hasReadOnlyOrGetterSetterPropertiesExcludingProto flag, we can't combine it with hasNonConfigurableProperties because MobX installs a non-writable administration property even on arrays, which have non-configurable "length" already, so combining them would disable optimization for JSArray. 3. Introduces HasStructureWithFlags bytecode op to be utilized in ProxyObject IC helpers to skip trap result validation for most of ProxyObject's targets. In the follow-ups, this approach will be expanded to C++ code. Currently, due to lack of proper profiling of GetInternalField results, constant folding for HasStructureWithFlags doesn't work in microbenchmarks. ToT patch proxy-has-miss 159.9825+-2.4190 ^ 43.1435+-0.5994 ^ definitely 3.7082x faster proxy-set-miss-handler 158.7617+-2.3615 ^ 139.2158+-1.9197 ^ definitely 1.1404x faster proxy-set 125.5967+-1.3678 ^ 14.4533+-0.1739 ^ definitely 8.6898x faster proxy-get 115.8061+-2.2413 ^ 14.2136+-0.1138 ^ definitely 8.1475x faster <geometric> 138.5985+-0.9526 ^ 33.3237+-0.2255 ^ definitely 4.1592x faster [1]: https://tc39.es/ecma262/#sec-invariants-of-the-essential-internal-methods * Source/JavaScriptCore/builtins/BuiltinNames.h: * Source/JavaScriptCore/builtins/ProxyHelpers.js: (linkTimeConstant.performProxyObjectHas): (linkTimeConstant.performProxyObjectGet): (linkTimeConstant.performProxyObjectSetSloppy): (linkTimeConstant.performProxyObjectSetStrict): * Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h: * Source/JavaScriptCore/bytecode/BytecodeList.rb: * Source/JavaScriptCore/bytecode/BytecodeUseDef.cpp: (JSC::computeUsesForBytecodeIndexImpl): (JSC::computeDefsForBytecodeIndexImpl): * Source/JavaScriptCore/bytecode/LinkTimeConstant.h: * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::emitHasStructureWithFlags): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: * Source/JavaScriptCore/bytecompiler/NodesCodegen.cpp: (JSC::BytecodeIntrinsicNode::emit_intrinsic_mustValidateResultOfProxyGetAndSetTraps): (JSC::BytecodeIntrinsicNode::emit_intrinsic_mustValidateResultOfProxyTrapsExceptGetAndSet): * Source/JavaScriptCore/dfg/DFGAbstractInterpreterInlines.h: (JSC::DFG::AbstractInterpreter<AbstractStateType>::executeEffects): * Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp: (JSC::DFG::ByteCodeParser::parseBlock): * Source/JavaScriptCore/dfg/DFGClobberize.h: (JSC::DFG::clobberize): * Source/JavaScriptCore/dfg/DFGConstantFoldingPhase.cpp: (JSC::DFG::ConstantFoldingPhase::foldConstants): * Source/JavaScriptCore/dfg/DFGDoesGC.cpp: (JSC::DFG::doesGC): * Source/JavaScriptCore/dfg/DFGFixupPhase.cpp: (JSC::DFG::FixupPhase::fixupNode): * Source/JavaScriptCore/dfg/DFGHeapLocation.cpp: (WTF::printInternal): * Source/JavaScriptCore/dfg/DFGHeapLocation.h: * Source/JavaScriptCore/dfg/DFGNode.h: (JSC::DFG::Node::structureFlags): * Source/JavaScriptCore/dfg/DFGNodeType.h: * Source/JavaScriptCore/dfg/DFGPredictionPropagationPhase.cpp: * Source/JavaScriptCore/dfg/DFGSafeToExecute.h: (JSC::DFG::safeToExecute): * Source/JavaScriptCore/dfg/DFGSpeculativeJIT.cpp: * Source/JavaScriptCore/dfg/DFGSpeculativeJIT.h: * Source/JavaScriptCore/dfg/DFGSpeculativeJIT32_64.cpp: (JSC::DFG::SpeculativeJIT::compile): * Source/JavaScriptCore/dfg/DFGSpeculativeJIT64.cpp: (JSC::DFG::SpeculativeJIT::compile): * Source/JavaScriptCore/ftl/FTLCapabilities.cpp: (JSC::FTL::canCompile): * Source/JavaScriptCore/ftl/FTLLowerDFGToB3.cpp: (JSC::FTL::DFG::LowerDFGToB3::compileNode): (JSC::FTL::DFG::LowerDFGToB3::compileCompareStrictEq): * Source/JavaScriptCore/jit/JIT.cpp: (JSC::JIT::privateCompileMainPass): * Source/JavaScriptCore/jit/JIT.h: * Source/JavaScriptCore/jit/JITOpcodes.cpp: (JSC::JIT::emit_op_has_structure_with_flags): * Source/JavaScriptCore/llint/LowLevelInterpreter32_64.asm: * Source/JavaScriptCore/llint/LowLevelInterpreter64.asm: * Source/JavaScriptCore/runtime/JSGlobalObject.cpp: (JSC::JSGlobalObject::init): * Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.cpp: (JSC::JSC_DEFINE_HOST_FUNCTION): (JSC::globalFuncHandleProxySetTrapResult): Deleted. * Source/JavaScriptCore/runtime/JSGlobalObjectFunctions.h: * Source/JavaScriptCore/runtime/JSTypeInfo.h: (JSC::TypeInfo::overridesIsExtensible const): * Source/JavaScriptCore/runtime/PropertySlot.h: * Source/JavaScriptCore/runtime/ProxyObject.cpp: (JSC::ProxyObject::performPut): (JSC::ProxyObject::validatePositiveSetTrapResult): (JSC::ProxyObject::validateSetTrapResult): Deleted. * Source/JavaScriptCore/runtime/ProxyObject.h: * Source/JavaScriptCore/runtime/Structure.cpp: (JSC::Structure::validateFlags): (JSC::Structure::Structure): (JSC::Structure::nonPropertyTransitionSlow): * Source/JavaScriptCore/runtime/Structure.h: (JSC::Structure::isNonExtensibleOrHasNonConfigurableProperties const): (JSC::Structure::bitField const): * Source/JavaScriptCore/runtime/StructureInlines.h: (JSC::Structure::add): (JSC::Structure::attributeChange): * Source/JavaScriptCore/wasm/js/WebAssemblyGCObjectBase.h: * Source/WebCore/bindings/js/JSDOMWindowProperties.h: Canonical link: https://commits.webkit.org/263443@main
1 parent 8fa47d3 commit 18d9fef

53 files changed

Lines changed: 308 additions & 65 deletions

Some content is hidden

Large Commits have some content hidden by default. Use the searchbox below for content that may be hidden.

‎JSTests/microbenchmarks/proxy-get-miss-handler.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,4 +1,6 @@
1+
(function() {
12
var proxy = new Proxy({}, {});
23

34
for (var i = 0; i < 1e7; ++i)
45
proxy.test;
6+
})();

‎JSTests/microbenchmarks/proxy-get.js‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,3 +1,4 @@
1+
(function() {
12
var proxy = new Proxy({}, {
23
get(target, propertyName, receiver) {
34
return 42;
@@ -6,3 +7,4 @@ var proxy = new Proxy({}, {
67

78
for (var i = 0; i < 1e7; ++i)
89
proxy.test;
10+
})();

‎JSTests/microbenchmarks/proxy-has-hit.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,6 @@
66
});
77

88
var has;
9-
for (var i = 0; i < 1e6; ++i)
9+
for (var i = 0; i < 1e7; ++i)
1010
has = "test" in proxy;
1111
})();

‎JSTests/microbenchmarks/proxy-has-miss-handler.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,6 @@
22
var proxy = new Proxy({}, {});
33

44
var has;
5-
for (var i = 0; i < 1e6; ++i)
5+
for (var i = 0; i < 1e7; ++i)
66
has = "test" in proxy;
77
})();

‎JSTests/microbenchmarks/proxy-has-miss.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -6,6 +6,6 @@
66
});
77

88
var has;
9-
for (var i = 0; i < 1e6; ++i)
9+
for (var i = 0; i < 1e7; ++i)
1010
has = "test" in proxy;
1111
})();

‎JSTests/microbenchmarks/proxy-set-miss-handler.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,6 @@
11
(function() {
22
var proxy = new Proxy({}, {});
33

4-
for (var i = 0; i < 1e6; ++i)
4+
for (var i = 0; i < 1e7; ++i)
55
proxy.test = i;
66
})();

‎JSTests/microbenchmarks/proxy-set.js‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,6 @@
55
}
66
});
77

8-
for (var i = 0; i < 1e6; ++i)
8+
for (var i = 0; i < 1e7; ++i)
99
proxy.test = i;
1010
})();

‎Source/JavaScriptCore/builtins/BuiltinNames.h‎

Lines changed: 1 addition & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -182,9 +182,8 @@ namespace JSC {
182182
macro(stringSplitFast) \
183183
macro(stringSubstring) \
184184
macro(handleNegativeProxyHasTrapResult) \
185+
macro(handlePositiveProxySetTrapResult) \
185186
macro(handleProxyGetTrapResult) \
186-
macro(handleProxySetTrapResultSloppy) \
187-
macro(handleProxySetTrapResultStrict) \
188187
macro(importModule) \
189188
macro(copyDataProperties) \
190189
macro(meta) \

‎Source/JavaScriptCore/builtins/ProxyHelpers.js‎

Lines changed: 13 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -44,7 +44,9 @@ function performProxyObjectHas(propertyName)
4444
if (trap.@call(handler, target, propertyName))
4545
return true;
4646

47-
@handleNegativeProxyHasTrapResult(target, propertyName);
47+
if (@mustValidateResultOfProxyTrapsExceptGetAndSet(target))
48+
@handleNegativeProxyHasTrapResult(target, propertyName);
49+
4850
return false;
4951
}
5052

@@ -68,9 +70,8 @@ function performProxyObjectGet(propertyName, receiver)
6870

6971
var trapResult = trap.@call(handler, target, propertyName, receiver);
7072

71-
// FIXME: Add op_get_own_property bytecode and IC, which returns two values, value and attributes.
72-
// Then we can implement it fully in JS.
73-
@handleProxyGetTrapResult(trapResult, target, propertyName);
73+
if (@mustValidateResultOfProxyGetAndSetTraps(target))
74+
@handleProxyGetTrapResult(trapResult, target, propertyName);
7475

7576
return trapResult;
7677
}
@@ -95,9 +96,11 @@ function performProxyObjectSetSloppy(propertyName, receiver, value)
9596
if (!@isCallable(trap))
9697
@throwTypeError("'set' property of a Proxy's handler should be callable");
9798

98-
var trapResult = trap.@call(handler, target, propertyName, value, receiver);
99+
if (!trap.@call(handler, target, propertyName, value, receiver))
100+
return;
99101

100-
@handleProxySetTrapResultSloppy(trapResult, target, propertyName, value);
102+
if (@mustValidateResultOfProxyGetAndSetTraps(target))
103+
@handlePositiveProxySetTrapResult(target, propertyName, value);
101104
}
102105

103106
@linkTimeConstant
@@ -120,7 +123,9 @@ function performProxyObjectSetStrict(propertyName, receiver, value)
120123
if (!@isCallable(trap))
121124
@throwTypeError("'set' property of a Proxy's handler should be callable");
122125

123-
var trapResult = trap.@call(handler, target, propertyName, value, receiver);
126+
if (!trap.@call(handler, target, propertyName, value, receiver))
127+
@throwTypeError("Proxy object's 'set' trap returned falsy value for property '" + @String(propertyName) + "'");
124128

125-
@handleProxySetTrapResultStrict(trapResult, target, propertyName, value);
129+
if (@mustValidateResultOfProxyGetAndSetTraps(target))
130+
@handlePositiveProxySetTrapResult(target, propertyName, value);
126131
}

‎Source/JavaScriptCore/bytecode/BytecodeIntrinsicRegistry.h‎

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,8 @@ enum class LinkTimeConstant : int32_t;
9898
macro(toString) \
9999
macro(toPropertyKey) \
100100
macro(toObject) \
101+
macro(mustValidateResultOfProxyGetAndSetTraps) \
102+
macro(mustValidateResultOfProxyTrapsExceptGetAndSet) \
101103
macro(newArrayWithSize) \
102104
macro(newArrayWithSpecies) \
103105
macro(newPromise) \

0 commit comments

Comments
 (0)