Bug 213193 – [JSC] add machinery to disable JIT tiers when experimental features are enabled

• Status: RESOLVED
• Resolution: FIXED
• Component: New Bugs

[Caitlin Potter (:caitp)]

[JSC] add machinery to disable JIT tiers when experimental features are enabled

[Caitlin Potter (:caitp)]

Created attachment 401894 [details]
Patch

[Mark Lam]

Comment on attachment 401894 [details]
Patch

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

r=me

> Source/JavaScriptCore/runtime/OptionsList.h:562
> +    BaselineOnly = 0,
> +    SupportsDFG = 1,
> +    SupportsFTL = 2,

Can you express the values as 1 << 0, 1 << 1, 1 << 2 instead?  That will make it clearer that these will be used as bit masks.  Let's also rename BaselineOnly to SupportsBaseline because it is conceivable that an experimental feature will be landed with LLInt only support.

Or do we want to maintain a minimum requirement that LLInt and Baseline be supported before an experimental feature can be turned on at all?  Since we do test with LLInt only and Baseline w/o LLInt, it might be easier to go with LLIntAndBaselineOnly as the minimum so that we'll have less Options to check.  If you prefer this route, please rename BaselineOnly to LLIntAndBaselineOnly, and keep its values 0, and express the other two as 1 << 0, and 1 << 1.  I think this might be the better way to go.

[Caitlin Potter (:caitp)]

Created attachment 401903 [details]
Patch For Landing

I have been treating _LLInt_ and _JIT_ as both being _baseline_ in my mind, but I can see how it's reasonable to distinguish between them. For now though, I'll opt to rename and treat them as the same _level_ of support, and if it's needed down the line, we can separate it then.

[Mark Lam]

Comment on attachment 401903 [details]
Patch For Landing

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

r=me

> Source/JavaScriptCore/runtime/Options.cpp:526
> +        } while (0);

nit: Use "while (false)" instead.

[Caitlin Potter (:caitp)]

Comment on attachment 401903 [details]
Patch For Landing

>Subversion Revision: 263033
>diff --git a/Source/JavaScriptCore/ChangeLog b/Source/JavaScriptCore/ChangeLog
>index eeb9f10289b874afd904644128a17482be211838..35db9e2c890c531931810e1e63dbc74732da0d72 100644
>--- a/Source/JavaScriptCore/ChangeLog
>+++ b/Source/JavaScriptCore/ChangeLog
>@@ -1,3 +1,23 @@
>[phone]  Caitlin Potter  <[e-mail]>
>+
>+        [JSC] add machinery to disable JIT tiers when experimental features are enabled
>+        https://bugs.webkit.org/show_bug.cgi?id=213193
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        A new macro FOR_EACH_JSC_EXPERIMENTAL_OPTION() supplies flags indicating the supported
>+        JIT tiers (or, in the future, other options) of a particular feature,
>+        in an easy to understand format. These flags are then used to
>+        recompute dependent feature flags.
>+
>+        This should simplify the incremental development of language features.
>+
>+        * dfg/DFGCapabilities.cpp:
>+        (JSC::DFG::capabilityLevel):
>+        * runtime/Options.cpp:
>+        (JSC::recomputeDependentOptions):
>+        * runtime/OptionsList.h:
>+
> 2020-06-13  Devin Rousso  <[e-mail]>
> 
>         Make `errors` an own property of `AggregateError` instead of a prototype accessor
>diff --git a/Source/JavaScriptCore/dfg/DFGCapabilities.cpp b/Source/JavaScriptCore/dfg/DFGCapabilities.cpp
>index 73101c7b59fd6ff32eda9a6e669931fd60d126a7..bd177b6557031060615b8a76c6b911aa32677c7d 100644
>--- a/Source/JavaScriptCore/dfg/DFGCapabilities.cpp
>+++ b/Source/JavaScriptCore/dfg/DFGCapabilities.cpp
>@@ -290,6 +290,7 @@ CapabilityLevel capabilityLevel(OpcodeID opcodeID, CodeBlock* codeBlock, const I
>     case op_unreachable:
>     case op_super_sampler_begin:
>     case op_super_sampler_end:
>+    case op_get_private_name:
>         return CanCompileAndInline;
> 
>     case op_switch_string: // Don't inline because we don't want to copy string tables in the concurrent JIT.
>@@ -298,7 +299,6 @@ CapabilityLevel capabilityLevel(OpcodeID opcodeID, CodeBlock* codeBlock, const I
> 
>     case op_yield:
>     case op_create_generator_frame_environment:
>-    case op_get_private_name: // FIXME: op_get_private_name will need DFG/FTL support to ship class fields. (https://bugs.webkit.org/show_bug.cgi?id=212781)
>     case llint_program_prologue:
>     case llint_eval_prologue:
>     case llint_module_program_prologue:
>diff --git a/Source/JavaScriptCore/runtime/Options.cpp b/Source/JavaScriptCore/runtime/Options.cpp
>index 796a836668a2e8e54463207dc246000a64185d77..e542b76fc481ebe20adc7cbe7cad371c096ea002 100644
>--- a/Source/JavaScriptCore/runtime/Options.cpp
>+++ b/Source/JavaScriptCore/runtime/Options.cpp
>@@ -514,6 +514,19 @@ static void recomputeDependentOptions()
>         Options::randomIntegrityAuditRate() = 0;
>     else if (Options::randomIntegrityAuditRate() > 1.0)
>         Options::randomIntegrityAuditRate() = 1.0;
>+
>+    if (!Options::allowUnsupportedTiers()) {
>+#define DISABLE_TIERS(option, flags, ...) do { \
>+            if (!Options::option())            \
>+                break;                         \
>+            if (!(flags & SupportsDFG))        \
>+                Options::useDFGJIT() = false;  \
>+            if (!(flags & SupportsFTL))        \
>+                Options::useFTLJIT() = false;  \
>+        } while (false);
>+
>+        FOR_EACH_JSC_EXPERIMENTAL_OPTION(DISABLE_TIERS);
>+    }
> }
> 
> inline void* Options::addressOfOption(Options::ID id)
>diff --git a/Source/JavaScriptCore/runtime/OptionsList.h b/Source/JavaScriptCore/runtime/OptionsList.h
>index a38caef593053b1cea6bf59aec4b7d250a4705ef..d5feb2045011c5ba39dba208d04997f59f0c44eb 100644
>--- a/Source/JavaScriptCore/runtime/OptionsList.h
>+++ b/Source/JavaScriptCore/runtime/OptionsList.h
>@@ -511,6 +511,7 @@ constexpr bool enableWebAssemblyStreamingApi = false;
>     v(Bool, usePublicClassFields, true, Normal, "If true, the parser will understand public data fields inside classes.") \
>     v(Bool, useRandomizingExecutableIslandAllocation, false, Normal, "For the arm64 ExecutableAllocator, if true, select which region to use randomly. This is useful for testing that jump islands work.") \
>     v(Bool, exposeProfilersOnGlobalObject, false, Normal, "If true, we will expose functions to enable/disable both the sampling profiler and the super sampler") \
>+    v(Bool, allowUnsupportedTiers, false, Normal, "If true, we will not disable DFG or FTL when an experimental feature is enabled.") \
>     v(Bool, usePrivateClassFields, false, Normal, "If true, the parser will understand private data fields inside classes.") \
> 
> enum OptionEquivalence {
>@@ -553,8 +554,16 @@ enum OptionEquivalence {
>     v(maximumFunctionForClosureCallInlineCandidateInstructionCount, maximumFunctionForClosureCallInlineCandidateBytecodeCost, SameOption) \
>     v(maximumFunctionForConstructInlineCandidateInstructionCount, maximumFunctionForConstructInlineCandidateBytecoodeCost, SameOption) \
>     v(maximumFTLCandidateInstructionCount, maximumFTLCandidateBytecodeCost, SameOption) \
>-    v(maximumInliningCallerSize, maximumInliningCallerBytecodeCost, SameOption) \
>+    v(maximumInliningCallerSize, maximumInliningCallerBytecodeCost, SameOption)
> 
>+enum ExperimentalOptionFlags {
>+    LLIntAndBaselineOnly = 0,
>+    SupportsDFG = 1 << 0,
>+    SupportsFTL = 1 << 1,
>+};
>+
>+#define FOR_EACH_JSC_EXPERIMENTAL_OPTION(v) \
>+    v(usePrivateClassFields, LLIntAndBaselineOnly, "https://bugs.webkit.org/show_bug.cgi?id=212781", "https://bugs.webkit.org/show_bug.cgi?id=212784")
> 
> constexpr size_t countNumberOfJSCOptions()
> {
>diff --git a/JSTests/ChangeLog b/JSTests/ChangeLog
>index 6040ad54fd73d1839624d39dc291e6b241c49aca..de7d418f6367845ae88def98603e689c98e355fe 100644
>--- a/JSTests/ChangeLog
>+++ b/JSTests/ChangeLog
>@@ -1,3 +1,14 @@
>[phone]  Caitlin Potter  <[e-mail]>
>+
>+        [JSC] add machinery to disable JIT tiers when experimental features are enabled
>+        https://bugs.webkit.org/show_bug.cgi?id=213193
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * stress/get-private-name.js:
>+        * stress/put-by-val-direct-addprivate.js:
>+        * stress/put-by-val-direct-putprivate.js:
>+
> 2020-06-14  Yusuke Suzuki  <[e-mail]>
> 
>         Add wasm regresion test for loop
>diff --git a/JSTests/stress/get-private-name.js b/JSTests/stress/get-private-name.js
>index 6f947f5d5db61aa2b234d39384b4ec8790e59aaf..63f4a25e24cb0a81fc8a6522462afef1a941aebb 100644
>--- a/JSTests/stress/get-private-name.js
>+++ b/JSTests/stress/get-private-name.js
>@@ -1,6 +1,4 @@
>-// FIXME: //@ requireOptions("--usePrivateClassFields=1") --- Run this in all variants once https://bugs.webkit.org/show_bug.cgi?id=212781 is fixed
>-//@ runNoJIT("--usePrivateClassFields=1")
>-//@ runNoLLInt("--usePrivateClassFields=1")
>+//@ requireOptions("--usePrivateClassFields=1")
> 
> // GetPrivateName should throw when the receiver does not have the requested private property
> let i, threw = false;
>diff --git a/JSTests/stress/put-by-val-direct-addprivate.js b/JSTests/stress/put-by-val-direct-addprivate.js
>index 931354686927f36ced9a99aee74491d8f7f23374..8c4e4d225403ccd5fae4049fdd56879455e3ce0b 100644
>--- a/JSTests/stress/put-by-val-direct-addprivate.js
>+++ b/JSTests/stress/put-by-val-direct-addprivate.js
>@@ -1,7 +1,4 @@
>-// TODO: //@ requireOptions("--usePrivateClassFields=1") -- Currently, eager JIT is not supported for private field access.
>-//@ runDefault("--usePrivateClassFields=1")
>-//@ runNoJIT("--usePrivateClassFields=1")
>-//@ runNoLLInt("--usePrivateClassFields=1")
>+//@ requireOptions("--usePrivateClassFields=1")
> 
> // PrivateField "Create" access should throw if writing to a non-existent PrivateName.
> let c, i = 0, threw = false;
>diff --git a/JSTests/stress/put-by-val-direct-putprivate.js b/JSTests/stress/put-by-val-direct-putprivate.js
>index b730b475949d4321e97e7917b2eb10ffd87ca27f..a730532da6af8782df6f3b8d86983f640f3296e0 100644
>--- a/JSTests/stress/put-by-val-direct-putprivate.js
>+++ b/JSTests/stress/put-by-val-direct-putprivate.js
>@@ -1,7 +1,4 @@
>-// FIXME: //@ requireOptions("--usePrivateClassFields=1") -- Currently, eager JIT is not supported for private field access. https://bugs.webkit.org/show_bug.cgi?id=212784
>-//@ runDefault("--usePrivateClassFields=1")
>-//@ runNoJIT("--usePrivateClassFields=1")
>-//@ runNoLLInt("--usePrivateClassFields=1")
>+//@ requireOptions("--usePrivateClassFields=1")
> 
> // PrivateField "Put" access should throw if writing to a non-existent PrivateName.
> let c, i = 0, threw = false;

[Caitlin Potter (:caitp)]

I have no idea what "edit as comment" is supposed to do, but that wasn't what I was expecting :)

[Caitlin Potter (:caitp)]

Created attachment 401911 [details]
Patch for landing

[EWS]

Committed r263046: <https://trac.webkit.org/changeset/263046>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 401911 [details].

[Radar WebKit Bug Importer]

<rdar://problem/64372319>