[JSC] move function wrapping logic to a new Function type -- WIP #2

caitp · 2021-11-29T16:19:13Z

In this initial patch, there is still a lot of JS-builtin machinery,
including some duplicated functionality. Additionally, JIT support
has not been incorporated yet.

Broadly, the idea is that there are custom hooks for calling a
JSRemoteFunction, which perform the wrapping functionality. This avoids
the need for allocating closures which contain the wrapping logic.

basic runtime
JIT/DFG/FTL support
structure caching (unnecessary since these are not constructors?)
improved baseline perf

In this initial patch, there is still a lot of JS-builtin machinery, including some duplicated functionality. Additionally, JIT support has not been incorporated yet. Broadly, the idea is that there are custom hooks for calling a JSRemoteFunction, which perform the wrapping functionality. This avoids the need for allocating closures which contain the wrapping logic. TODO: - JIT/DFG/FTL support - structure caching (unnecessary since these are not constructors?) - improved baseline perf

Source/JavaScriptCore/runtime/JSRemoteFunction.cpp

caitp · 2021-11-29T16:24:11Z

Source/JavaScriptCore/runtime/JSRemoteFunction.h

+
+    JS_EXPORT_PRIVATE static JSRemoteFunction* create(VM&, JSGlobalObject*, JSCallee* targetFunction);
+
+    JSCallee* targetFunction() { return m_targetFunction.get(); }


TODO: helpers to access the global object of the target function's realm and wrapper function's realm?

caitp · 2021-11-29T16:24:38Z

Source/JavaScriptCore/runtime/VM.cpp

+            return cached;
+        NativeExecutable* result = getHostFunction(
+            slowCase ? remoteFunctionCall : remoteJSFunctionCall,
+            // FIXME: Add thunk generator for FastRemoteFunctionCall


need this JIT support to be comparable to JSBoundFunction

caitp · 2021-11-29T16:25:37Z

Source/JavaScriptCore/runtime/JSRemoteFunction.cpp

+    auto scope = DECLARE_THROW_SCOPE(vm);
+    JSValue result = wrapValue(globalObject, targetGlobalObject, value);
+    if (result.isEmpty())
+        throwVMError(globalObject, scope, createTypeError(globalObject, "value passing between realms must be callable or primitive"));


TODO: return value specific error message?

caitp · 2021-11-29T16:26:01Z

Source/JavaScriptCore/bytecode/LinkTimeConstant.h

    v(emptyPropertyNameEnumerator, nullptr) \
    v(sentinelString, nullptr) \
+    v(createRemoteFunction, nullptr) \
+    v(isRemoteFunction, nullptr) \


TODO: use isRemoteFunction in tests

…tly)

caitp · 2022-07-23T18:10:10Z

This landed as part of WebKit@b1defcb, and can be closed

…a rejected promise https://bugs.webkit.org/show_bug.cgi?id=247785 rdar://102325201 Reviewed by Yusuke Suzuki. Rest parameter should be caught in async function. So, running this JavaScript program should print "caught". ``` async function f(...[[]]) { } f().catch(e => print("caught")); ``` V8 (used console.log) ``` $ node input.js caught ``` GraalJS ``` $ js input.js caught ``` https://tc39.es/ecma262/#sec-async-function-definitions ... AsyncFunctionDeclaration[Yield, Await, Default] : async [no LineTerminator here] function BindingIdentifier[?Yield, ?Await] ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } [+Default] async [no LineTerminator here] function ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } AsyncFunctionExpression : async [no LineTerminator here] function BindingIdentifier[~Yield, +Await]opt ( FormalParameters[~Yield, +Await] ) { AsyncFunctionBody } ... According to the spec, it indicates `FormalParameters` is used for Async Function, where `FormalParameters` can be converted to `FunctionRestParameter`. https://tc39.es/ecma262/#sec-parameter-lists ... FormalParameters[Yield, Await] : [empty] FunctionRestParameter[?Yield, ?Await] FormalParameterList[?Yield, ?Await] FormalParameterList[?Yield, ?Await] , FormalParameterList[?Yield, ?Await] , FunctionRestParameter[?Yield, ?Await] ... And based on RS: EvaluateAsyncFunctionBody, it will invoke the promise.reject callback function with abrupt value ([[value]] of non-normal completion record). https://tc39.es/ecma262/#sec-runtime-semantics-evaluateasyncfunctionbody ... 2. Let declResult be Completion(FunctionDeclarationInstantiation(functionObject, argumentsList)). 3. If declResult is an abrupt completion, then a. Perform ! Call(promiseCapability.[[Reject]], undefined, « declResult.[[Value]] »). ... In that case, any non-normal results of evaluating rest parameters should be caught and passed to the reject callback function. To resolve this problem, we should allow the emitted RestParameterNode be wrapped by the catch handler for promise. However, we should remove `m_restParameter` and emit rest parameter byte code in `initializeDefaultParameterValuesAndSetupFunctionScopeStack` if we can prove that change has no side effect. In that case, we can only use one exception handler. Current fix is to add another exception handler. And move the handler byte codes to the bottom of code block in order to make other byte codes as much compact as possible. Input: ``` async function f(arg0, ...[[]]) { } f(); ``` Dumped Byte Codes: ``` ... bb#2 Predecessors: [ #1 ] [ 20] mov dst:loc9, src:<JSValue()>(const0) ... bb#3 Predecessors: [ #2 ] [ 29] get_rest_length dst:loc11, numParametersToSkip:1 ... bb#12 Predecessors: [ WebKit#8 WebKit#9 WebKit#10 ] [ 138] new_func_exp dst:loc10, scope:loc4, functionDecl:0 ... bb#13 Predecessors: [ ] [ 170] catch exception:loc10, thrownValue:loc8 [ 174] jmp targetLabel:8(->182) Successors: [ WebKit#15 ] bb#14 Predecessors: [ WebKit#7 WebKit#11 ] [ 176] catch exception:loc10, thrownValue:loc8 [ 180] jmp targetLabel:2(->182) Successors: [ WebKit#15 ] bb#15 Predecessors: [ WebKit#13 WebKit#14 ] [ 182] mov dst:loc12, src:Undefined(const1) ... Exception Handlers: 1: { start: [ 20] end: [ 29] target: [ 170] } synthesized catch 2: { start: [ 29] end: [ 138] target: [ 176] } synthesized catch ``` * JSTests/stress/catch-rest-parameter.js: Added. (throwError): (shouldThrow): (async f): (throwError.async f): (throwError.async let): (async let): (x.async f): (x): (async shouldThrow): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.cpp: (JSC::BytecodeGenerator::BytecodeGenerator): (JSC::BytecodeGenerator::initializeDefaultParameterValuesAndSetupFunctionScopeStack): * Source/JavaScriptCore/bytecompiler/BytecodeGenerator.h: Canonical link: https://commits.webkit.org/256864@main

https://bugs.webkit.org/show_bug.cgi?id=248506 Reviewed by Antti Koivisto. 1. Do not set "layout bounds" on non-inline boxes (spec agrees here) 2. Make sure hard line breaks still stretch their parent inline boxes when applicable Current behavior: 1. Line break box gets computed layout bounds 2. When the line break box affects the line box, we stretch the line by the computed layout bounds value Patch behavior: 1. Line break box makes the parent inline box "contentful" when applicable (this is the same as #2 at current behavior) 2. The "contentful" inline box (mostly the root inline box) stretches the line box by the computed layout bounds. It makes break box behave like regular "text content", where the content indirectly affects the line box height. This patch is also in preparation for supporting text-edge, where text-edge affects the layout bounds value. * Source/WebCore/layout/formattingContexts/inline/InlineFormattingGeometry.cpp: (WebCore::Layout::InlineFormattingGeometry::inlineLevelBoxAffectsLineBox const): * Source/WebCore/layout/formattingContexts/inline/InlineFormattingGeometry.h: * Source/WebCore/layout/formattingContexts/inline/InlineFormattingQuirks.cpp: (WebCore::Layout::InlineFormattingQuirks::lineBreakBoxAffectsParentInlineBox): (WebCore::Layout::InlineFormattingQuirks::inlineBoxAffectsLineBox const): (WebCore::Layout::InlineFormattingQuirks::inlineLevelBoxAffectsLineBox const): Deleted. * Source/WebCore/layout/formattingContexts/inline/InlineFormattingQuirks.h: * Source/WebCore/layout/formattingContexts/inline/InlineLineBoxBuilder.cpp: (WebCore::Layout::LineBoxBuilder::setVerticalPropertiesForInlineLevelBox const): (WebCore::Layout::LineBoxBuilder::constructInlineLevelBoxes): (WebCore::Layout::LineBoxBuilder::adjustIdeographicBaselineIfApplicable): * Source/WebCore/layout/formattingContexts/inline/InlineLineBoxVerticalAligner.cpp: (WebCore::Layout::LineBoxVerticalAligner::computeLineBoxLogicalHeight const): (WebCore::Layout::LineBoxVerticalAligner::computeRootInlineBoxVerticalPosition const): Canonical link: https://commits.webkit.org/257288@main

https://bugs.webkit.org/show_bug.cgi?id=250196 rdar://98798050 Reviewed by Simon Fraser and Dean Jackson. WebKit has long accidentally depended on the combination of two somewhat unusual behavioral quirks in CGIOSurfaceContext: 1) (Source) If you make a CGImageRef from one CGIOSurfaceContext via CGIOSurfaceContextCreateImage, and mutate the original IOSurface under the hood (or in a different process) in a way that CGIOSurfaceContext does not know, CGIOSurfaceContextCreateImage will return the same CGImageRef when called later. 2) (Destination) If you make a CGImageRef from one CGIOSurfaceContext via CGIOSurfaceContextCreateImage, paint it into a different CGIOSurfaceContext, then mutate the original IOSurface, and paint the same CGImageRef again, the updated IOSurface contents will be used the second time. The second quirk has never worked with unaccelerated CoreGraphics bitmap context destinations. Instead, in the unaccelerated case, the CGImageRef acts as a snapshot of the surface at the time it was created. We've long had code to handle this, forcing CGIOSurfaceContextCreateImage to re-create the CGImageRef each time we paint it (by drawing an empty rect into the CGIOSurfaceContext), working around quirk #1 and thus bypassing quirk #2, if we're painting into an unaccelerated backing store. It turns out our CG display list backing store implementation behaves like a CG bitmap context (without quirk #2), and so currently any IOSurfaces painted into CG display list backing store from a CGImageRef created by CGIOSurfaceContextCreateImage (but not -CreateImageReference) become stale if painted multiple times. To avoid this, extend the workaround to apply to any destination context that claims that it needs the workaround, and use it whenever painting an IOSurface into anything other than a CGIOSurfaceContext. * Source/WebCore/platform/graphics/BifurcatedGraphicsContext.cpp: (WebCore::BifurcatedGraphicsContext::needsCachedNativeImageInvalidationWorkaround): * Source/WebCore/platform/graphics/BifurcatedGraphicsContext.h: Make BifurcatedGraphicsContext assume the more conservative mode of its two children. * Source/WebCore/platform/graphics/GraphicsContext.h: (WebCore::GraphicsContext::needsCachedNativeImageInvalidationWorkaround): Assume that by default, GraphicsContexts need the workaround. * Source/WebCore/platform/graphics/cg/GraphicsContextCG.cpp: (WebCore::GraphicsContextCG::needsCachedNativeImageInvalidationWorkaround): * Source/WebCore/platform/graphics/cg/GraphicsContextCG.h: GraphicsContextCG needs the workaround, except in the IOSurface->IOSurface case. * Source/WebCore/platform/graphics/cg/ImageBufferIOSurfaceBackend.cpp: (WebCore::ImageBufferIOSurfaceBackend::finalizeDrawIntoContext): Confer with the GraphicsContext about its need for the workaround instead of hardcoding the behavior here. * Source/WebKit/Shared/RemoteLayerTree/CGDisplayListImageBufferBackend.mm: CG display list graphics contexts need the workaround. Canonical link: https://commits.webkit.org/258586@main

https://bugs.webkit.org/show_bug.cgi?id=251063 rdar://104585575 Reviewed by Mark Lam and Justin Michaud. This patch enhances CallFrame::dump to support wasm frames in btjs stacktrace. The example is as follows. frame #0: 0x00000001035fca78 JavaScriptCore`JSC::functionBreakpoint(globalObject=0x000000012f410068, callFrame=0x000000016fdfa9d0) at JSDollarVM.cpp:2273:9 [opt] frame #1: 0x000000010ec44204 0x10eccc5dc frame #2: 0x000000010eccc5dc callback#Dwaxn6 [Baseline bc#50](Undefined) frame WebKit#3: 0x000000010ec4ca84 wasm-stub [WasmToJS](Wasm::Instance: 0x10d29da40) frame WebKit#4: 0x000000010ed0c060 <?>.wasm-function[1] [OMG](Wasm::Instance: 0x10d29da40) frame WebKit#5: 0x000000010ed100d0 jsToWasm#CWTx6k [FTL bc#22](Cell[JSModuleEnvironment]: 0x12f524540, Cell[WebAssemblyFunction]: 0x10d06a3a8, 1, 2, 3) frame WebKit#6: 0x000000010ec881b0 #D5ymZE [Baseline bc#733](Undefined, Cell[Generator]: 0x12f55c180, 1, Cell[Object]: 0x12f69dfc0, 0, Cell[JSLexicalEnvironment]: 0x12f52cee0) frame WebKit#7: 0x000000010ec3c008 asyncFunctionResume#A4ayYg [LLInt bc#49](Undefined, Cell[Generator]: 0x12f55c180, Cell[Object]: 0x12f69dfc0, 0) frame WebKit#8: 0x000000010ec3c008 promiseReactionJobWithoutPromise#D0yDF1 [LLInt bc#25](Undefined, Cell[Function]: 0x12f44f3c0, Cell[Object]: 0x12f69dfc0, Cell[Generator]: 0x12f55c180) frame WebKit#9: 0x000000010ec80ec0 promiseReactionJob#EdShZz [Baseline bc#74](Undefined, Undefined, Cell[Function]: 0x12f44f3c0, Cell[Object]: 0x12f69dfc0, Cell[Generator]: 0x12f55c180) frame WebKit#10: 0x000000010ec3c728 frame WebKit#11: 0x0000000103137560 JavaScriptCore`JSC::Interpreter::executeCall(JSC::JSGlobalObject*, JSC::JSObject*, JSC::CallData const&, JSC::JSValue, JSC::ArgList const&) [inlined] JSC::JITCode::execute(this=<unavailable>, vm=<unavailable>, protoCallFrame=<unavailable>) at JITCodeInlines.h:42:38 [opt] frame WebKit#12: 0x0000000103137524 JavaScriptCore`JSC::Interpreter::executeCall(this=<unavailable>, lexicalGlobalObject=<unavailable>, function=<unavailable>, callData=<unavailable>, thisValue=<unavailable>, args=<unavailable>) at Interpreter.cpp:1093:27 [opt] frame WebKit#13: 0x000000010349d6d0 JavaScriptCore`JSC::runJSMicrotask(globalObject=0x000000012f410068, identifier=(m_identifier = 81), job=JSValue @ x22, argument0=JSValue @ x26, argument1=JSValue @ x25, argument2=<unavailable>, argument3=<unavailable>) at JSMicrotask.cpp:98:9 [opt] frame WebKit#14: 0x00000001039dfc54 JavaScriptCore`JSC::VM::drainMicrotasks() (.cold.1) at VM.cpp:0:9 [opt] frame WebKit#15: 0x00000001035e58a4 JavaScriptCore`JSC::VM::drainMicrotasks() [inlined] JSC::MicrotaskQueue::dequeue(this=<unavailable>) at VM.cpp:0:9 [opt] frame WebKit#16: 0x00000001035e5894 JavaScriptCore`JSC::VM::drainMicrotasks(this=0x000000012f000000) at VM.cpp:1255:46 [opt] ... * Source/JavaScriptCore/interpreter/CallFrame.cpp: (JSC::CallFrame::dump const): Canonical link: https://commits.webkit.org/259262@main

caitp commented Nov 30, 2021

View reviewed changes

caitp added 8 commits December 1, 2021 15:59

fixup: simplify not-a-constructor handling and throw better message

1a292e6

fixup: delete printf debugging

6bee57a

fixup: use throwTypeError instead of throwVMError(...createTypeError())

42215ec

thunkgenerators v1

32d3053

wip2

7cceeee

more comments

39b8d52

test that I can use the host function in a shadow realm (fails curren…

23e84bb

…tly)

testing various aspects of the code (mostly works, fails a bit)

50a4a5e

caitp closed this Jul 23, 2022

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[JSC] move function wrapping logic to a new Function type -- WIP #2

[JSC] move function wrapping logic to a new Function type -- WIP #2

Uh oh!

caitp commented Nov 29, 2021 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caitp Nov 29, 2021

Uh oh!

caitp Nov 29, 2021

Uh oh!

caitp Nov 29, 2021

Uh oh!

caitp Nov 29, 2021

Uh oh!

caitp commented Jul 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants


		JS_EXPORT_PRIVATE static JSRemoteFunction* create(VM&, JSGlobalObject, JSCallee targetFunction);

		JSCallee* targetFunction() { return m_targetFunction.get(); }

[JSC] move function wrapping logic to a new Function type -- WIP #2

[JSC] move function wrapping logic to a new Function type -- WIP #2

Uh oh!

Conversation

caitp commented Nov 29, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

caitp Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

caitp Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

caitp Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

caitp Nov 29, 2021

Choose a reason for hiding this comment

Uh oh!

caitp commented Jul 23, 2022

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

caitp commented Nov 29, 2021 •

edited

Loading