Bug 212556 - Make Options::useJIT() be the canonical source of truth on whether we should use the JIT.
Summary: Make Options::useJIT() be the canonical source of truth on whether we should ...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mark Lam
URL:
Keywords: InRadar
Depends on: 212561
Blocks:
  Show dependency treegraph
 
Reported: 2020-05-29 17:29 PDT by Mark Lam
Modified: 2020-06-16 16:52 PDT (History)
6 users (show)

See Also:


Attachments
work in progress. (13.41 KB, patch)
2020-05-29 17:33 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (17.24 KB, patch)
2020-05-29 23:56 PDT, Mark Lam
no flags Details | Formatted Diff | Diff
proposed patch. (43.94 KB, patch)
2020-06-16 02:31 PDT, Mark Lam
sbarati: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mark Lam 2020-05-29 17:29:58 PDT
We should make Options::useJIT() means what it says instead of having to rely on a separate VM::s_canUseJIT.
Comment 1 Radar WebKit Bug Importer 2020-05-29 17:30:37 PDT
<rdar://problem/63780436>
Comment 2 Mark Lam 2020-05-29 17:33:27 PDT
Created attachment 400640 [details]
work in progress.
Comment 3 Mark Lam 2020-05-29 23:56:43 PDT
Created attachment 400653 [details]
proposed patch.
Comment 4 Mark Lam 2020-05-30 09:37:08 PDT
Comment on attachment 400653 [details]
proposed patch.

Ready for a review.
Comment 5 Saam Barati 2020-05-30 09:51:25 PDT
Comment on attachment 400653 [details]
proposed patch.

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

> Source/JavaScriptCore/ChangeLog:3
> +        Make Options::useJIT() be the canonical source of truth on whether we should use the JITs.

I’m not sure I love the premise here. Options::useJIT kinda means two things now, one thing before, and one thing after, enabling the assembler

> Source/JavaScriptCore/runtime/Options.cpp:659
> +void Options::finalizeJITOptions()

A better strategy is to assert this is called once. Seems wrong if a second caller finds it necessary to do this

> Source/JavaScriptCore/runtime/VM.h:-807
> -        ASSERT(s_canUseJITIsSet);

Can we ASSERT still we’re asking this only after JIT options are set
Comment 6 Saam Barati 2020-05-30 09:54:15 PDT
Comment on attachment 400653 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/VM.h:-807
>> -        ASSERT(s_canUseJITIsSet);
> 
> Can we ASSERT still we’re asking this only after JIT options are set

Maybe this function should just be removed?
Comment 7 Saam Barati 2020-05-30 09:55:43 PDT
Comment on attachment 400653 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/Options.cpp:390
> +};

No semicolon
Comment 8 Mark Lam 2020-05-30 10:05:59 PDT
Comment on attachment 400653 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/Options.cpp:659
>> +void Options::finalizeJITOptions()
> 
> A better strategy is to assert this is called once. Seems wrong if a second caller finds it necessary to do this

Will do.

>>> Source/JavaScriptCore/runtime/VM.h:-807
>>> -        ASSERT(s_canUseJITIsSet);
>> 
>> Can we ASSERT still we’re asking this only after JIT options are set
> 
> Maybe this function should just be removed?

This should only be used after initializeThreading() (it was always this way or we would have fallen on our face).  As stated in my ChangeLog, removing this will cause a lot of code churn.  I opted to do that in a follow up refactoring patch if desired.

A few things to note:
1. If we want to assert that the JIT option has been finalized (which I can do), then we'll need to keep this function.
2. Checking Options::useJIT() is slightly more expensive than checking a boolean: it needs to load the offset of the boolean and then load the boolean.  This was made necessary because Clang apparently does not memoize constexpr function call reifications.  I previously had the Options offsets computed as constexpr function calls, but that made the build slow enough that people got mad.  So, this small runtime cost is the tradeoff made to keep people from complaining about build times.  That said, I don't think we check VM::canUseJIT() a lot and in perf critical paths.  So, it probably doesn't matter if we just check Options::useJIT() instead of pre-caching its value in a boolean.
Comment 9 Mark Lam 2020-05-30 10:07:32 PDT
Comment on attachment 400653 [details]
proposed patch.

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

>> Source/JavaScriptCore/runtime/Options.cpp:390
>> +};
> 
> No semicolon

Oops.  This used to be a lambda in my local code, but got moved out into a function.  Will fix.
Comment 10 Saam Barati 2020-06-03 18:45:10 PDT
Comment on attachment 400653 [details]
proposed patch.

Clearing r? for now
Comment 11 Mark Lam 2020-06-16 02:31:14 PDT
Created attachment 401990 [details]
proposed patch.
Comment 12 Michael Saboff 2020-06-16 10:35:30 PDT
I must be missing something.  How does Options::useJIT() check that we have the JIT entitlement like VM::canUseJIT() does via VM::canUseAssembler()?
Comment 13 Mark Lam 2020-06-16 10:44:08 PDT
(In reply to Michael Saboff from comment #12)
> I must be missing something.  How does Options::useJIT() check that we have
> the JIT entitlement like VM::canUseJIT() does via VM::canUseAssembler()?

Search for VM::computeCanUseJIT() in the patch.  It computes g_jscConfig.vm.canUseJIT just like it used to compute VM::s_canUseJIT.

The search for the check for !g_jscConfig.vm.canUseJIT in initializeThreading().  If !g_jscConfig.vm.canUseJIT, we set useJIT() to false.

In short, it works exactly as it does before except that we put the result in useJIT().  We still have a canUseJIT flag in g_jscConfig.vm.canUseJIT, but that is only used for the computation at initialization time to determine whether the JIT can be enabled or not.
Comment 14 Saam Barati 2020-06-16 11:42:00 PDT
Comment on attachment 401990 [details]
proposed patch.

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

> Source/JavaScriptCore/runtime/JSCConfig.h:72
> +    } vm;

why is this named "vm"?
Comment 15 Mark Lam 2020-06-16 11:43:15 PDT
Comment on attachment 401990 [details]
proposed patch.

Taking out of review.  Will look at a better abstraction for Options initialization before I propose another patch.
Comment 16 Mark Lam 2020-06-16 11:45:38 PDT
(In reply to Saam Barati from comment #14)
> Comment on attachment 401990 [details]
> proposed patch.
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=401990&action=review
> 
> > Source/JavaScriptCore/runtime/JSCConfig.h:72
> > +    } vm;
> 
> why is this named "vm"?

vm's group of data in the g_jscConfig.  Kind of a namespace grouping as opposed to have a giant bag of data all at the same level.  As we add more data into the Config, having some organization will help us better compartmentalize what's what in there.
Comment 17 Mark Lam 2020-06-16 12:17:08 PDT
Comment on attachment 401990 [details]
proposed patch.

Talked to folks offline.  Since they are ok with taking this patch as is for now, I'll defer the abstraction clean up for another patch later.  Putting back the r?.
Comment 18 Saam Barati 2020-06-16 12:54:57 PDT
Comment on attachment 401990 [details]
proposed patch.

r=me

Might be worth noting your clean up plan here for those not in the slack
Comment 19 Mark Lam 2020-06-16 14:14:49 PDT
(In reply to Saam Barati from comment #18)
> Comment on attachment 401990 [details]
> proposed patch.
> 
> r=me
> 
> Might be worth noting your clean up plan here for those not in the slack

OK, here are the details:

JSC Options have always meant 2 different things at different times:

Meaning 1: the set of requested behavior settings, which may or may not be adopted, and may be modified / adjusted based on various constraints.

Meaning 2: the set of finalized behavior settings, which is what will actually be adopted and is fixed at some finalization point.

VM::s_canUseJIT is from the Meaning 2 set, whereas we treated Options::useJIT() as also being from the Meaning 1 set.  However, all other Options just use the storage in Options to mean Meaning 1 at initialization time, and switches to Meaning 2 after the finalization point.

The finalization point was also always nebulous.  We just relied on all clients doing the right thing and using the Option value before it is finalized without any enforcement.  VM::canUseJIT() has enforcement by asserting on VM::s_canUseJITIsSet.

This patch makes it explicit that Options have 2 meanings.  It also makes the finalization point explicit i.e. when Options::finalize() is called.  Before Options::finalized(), Options values all have Meaning 1.  After Options::finalized(), they have meaning 2.  Clients that try to use them for meaning 2 will have to assert that g_jscConfig.options.isFinalized is true.

With this formalization, we no longer need VM::s_canUseJIT, which is a special case of this.

Ideally, we will clean this up even more later by giving the 2 sets of meanings 2 distinct abstractions to disambiguate between the 2.  Maybe call the first set RequestedOptions, or OptionArguments. And keep the Options name for the second set to minimize code change.  With that, we can also statically enforce that the second set is read only by clients.
Comment 20 Mark Lam 2020-06-16 14:30:57 PDT
Thanks for the reviews.  Landed in r263117: <http://trac.webkit.org/r263117>.