Bug 210920 - Improve API::SerializedScriptValue::deserialize to not allocate a new JSContext every second
Summary: Improve API::SerializedScriptValue::deserialize to not allocate a new JSConte...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit API (show other bugs)
Version: Safari 13
Hardware: All All
: P2 Enhancement
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2020-04-23 10:13 PDT by Simon Taylor
Modified: 2022-04-06 11:31 PDT (History)
12 users (show)

See Also:


Attachments
Patch (3.00 KB, patch)
2022-04-06 07:53 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Simon Taylor 2020-04-23 10:13:24 PDT
In r186229 (related to bug 146416) a static API::SerializedScriptValue::deserialize() function was introduced.

This function converts from a SerializedScriptValue to an Objective C object, and is currently used in the implementation of these APIs that return results from the content process:
 - evaluateJavaScript:completionHandler: for WKWebView, and
 - userContentController:didReceiveScriptMessage: for WKScriptMessageHandler

SerializedScriptValue::deserialize uses a JSContext to do the conversion, with a SharedJSContext helper to cache this between calls so it does not need to create a completely fresh one each time (this was bug 146416).

However the SharedJSContext clears the cached JSContext one second after its initial creation due to this line:
https://trac.webkit.org/browser/webkit/trunk/Source/WebKit/UIProcess/API/Cocoa/APISerializedScriptValueCocoa.mm#L49

Any apps that make heavy use of passing data from the WKWebView to the native side therefore end up recreating the JSContext for the Objective C value conversion once per second.

The most annoying implication of this is when debugging - this only-for-conversion JSContext shows up in Safari's Develop menu, and makes the options to automatically show inspector and pause JSContexts essentially useless. It was digging into what those were that led to this entire adventure :)

There is also a small performance impact - from Instruments System Trace it looks like my iPod 7 spends around 1.5ms doing [[JSContext alloc] init]. It's only at most once per second, so not huge overall, but still would be nice to get rid of this cost.

Here's my initial thoughts for improvements:
 - Reset the timeout to clear the shared JSContext in every ensureContext() call (so active use of evaluateJavascript will keep it around indefinitely)
 - Increment a ref count on the shared context for each WKScriptMessageHandler attached

I don't know the memory cost of the shared JSContext, but attaching a WKScriptMessageHandler is a pretty clear signal the developer expects messages to be coming back from the WebView, so I'd argue keeping the shared context around in this case makes sense.
Comment 1 chad 2022-03-19 03:16:18 PDT
Looks like a solution for this issue: WebKit::WebUserContentControllerProxy::didReceiveMessage spents long time on JSContext initailization. https://developer.apple.com/forums/thread/702626
Comment 2 Chris Dumez 2022-04-06 07:53:01 PDT
Created attachment 456819 [details]
Patch
Comment 3 Geoffrey Garen 2022-04-06 10:39:03 PDT
Comment on attachment 456819 [details]
Patch

r=me
Comment 4 EWS 2022-04-06 11:30:53 PDT
Committed r292483 (249334@main): <https://commits.webkit.org/249334@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 456819 [details].
Comment 5 Radar WebKit Bug Importer 2022-04-06 11:31:29 PDT
<rdar://problem/91366573>