Bug 194084 – [JSC] Do not use InferredValue in non-JIT configuration

• Status: RESOLVED
• Resolution: FIXED
• Component: JavaScriptCore

[Yusuke Suzuki]

This is typically used to identify that the given value is a singleton. And held value and watchpoint are used in DFG / FTL to make that value constant.
This is allocated for each JSFunction and SymbolTable. It is

1. Costly. Since this also requires IsoSubspace, IsoCellSet, and finalizers.
2. While this InferredValue is also used in ObjectAllocationProfile to check the possibility of poly proto, InlinedWatchpoint is enough for this use case.

[Yusuke Suzuki]

(In reply to Yusuke Suzuki from comment #0)
> This is typically used to identify that the given value is a singleton. And
> held value and watchpoint are used in DFG / FTL to make that value constant.
> This is allocated for each JSFunction and SymbolTable. It is
> 
> 1. Costly. Since this also requires IsoSubspace, IsoCellSet, and finalizers.
> 2. While this InferredValue is also used in ObjectAllocationProfile to check
> the possibility of poly proto, InlinedWatchpoint is enough for this use case.

Actually, in non-JIT configuration, WatchpointState is enough (InlineWatchpointSet is unnecessary).

[Yusuke Suzuki]

Created attachment 360701 [details]
Patch

[Yusuke Suzuki]

Comment on attachment 360701 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360701&action=review

> Source/JavaScriptCore/runtime/FunctionExecutable.cpp:61
> +    if (VM::canUseJIT())

Currently, we are using `VM::canUseJIT()` condition here. But we can change this to `VM::isInMiniMode()` if we have a null check in JIT code for `singletonFunction()`.
Either is OK to me.

[Yusuke Suzuki]

Created attachment 360702 [details]
Patch

[EWS Watchlist]

Comment on attachment 360702 [details]
Patch

Attachment 360702 [details] did not pass mac-ews (mac):
Output: https://webkit-queues.webkit.org/results/10966995

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360705 [details]
Archive of layout-test-results from ews103 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews103  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360702 [details]
Patch

Attachment 360702 [details] did not pass mac-debug-ews (mac):
Output: https://webkit-queues.webkit.org/results/10966942

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360706 [details]
Archive of layout-test-results from ews112 for mac-highsierra

The attached test failures were seen while running run-webkit-tests on the mac-debug-ews.
Bot: ews112  Port: mac-highsierra  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360702 [details]
Patch

Attachment 360702 [details] did not pass mac-wk2-ews (mac-wk2):
Output: https://webkit-queues.webkit.org/results/10967002

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360708 [details]
Archive of layout-test-results from ews106 for mac-highsierra-wk2

The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews.
Bot: ews106  Port: mac-highsierra-wk2  Platform: Mac OS X 10.13.6

[EWS Watchlist]

Comment on attachment 360702 [details]
Patch

Attachment 360702 [details] did not pass ios-sim-ews (ios-simulator-wk2):
Output: https://webkit-queues.webkit.org/results/10966994

Number of test failures exceeded the failure limit.

[EWS Watchlist]

Created attachment 360710 [details]
Archive of layout-test-results from ews121 for ios-simulator-wk2

The attached test failures were seen while running run-webkit-tests on the ios-sim-ews.
Bot: ews121  Port: ios-simulator-wk2  Platform: Mac OS X 10.13.6

[Yusuke Suzuki]

Created attachment 360711 [details]
Patch

[Saam Barati]

Comment on attachment 360711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360711&action=review

> Source/JavaScriptCore/ChangeLog:10
> +        is preferable target for poly proto. But in this case, we do not need to have full InferredValue. Just holding WatchpointState

It’s worth actually explaining what you’re doing here. You’re seeing if we ever create more than once. And you’re also saying only the JIT cares about the actual function being inferred

> Source/JavaScriptCore/runtime/FunctionExecutable.h:193
> +    void notifyWrite(VM& vm, JSValue value, const char* reason)

This name makes less sense in this context. You should rename it to make more sense. Maybe notifyCreation.

[Yusuke Suzuki]

Comment on attachment 360711 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=360711&action=review

>> Source/JavaScriptCore/ChangeLog:10
>> +        is preferable target for poly proto. But in this case, we do not need to have full InferredValue. Just holding WatchpointState
> 
> It’s worth actually explaining what you’re doing here. You’re seeing if we ever create more than once. And you’re also saying only the JIT cares about the actual function being inferred

Make sense. Described.

>> Source/JavaScriptCore/runtime/FunctionExecutable.h:193
>> +    void notifyWrite(VM& vm, JSValue value, const char* reason)
> 
> This name makes less sense in this context. You should rename it to make more sense. Maybe notifyCreation.

Nice, changed.

[Yusuke Suzuki]

Committed r240796: <https://trac.webkit.org/changeset/240796>

[Radar WebKit Bug Importer]

<rdar://problem/47709022>