We should make Options::useJIT() means what it says instead of having to rely on a separate VM::s_canUseJIT.
<rdar://problem/63780436>
Created attachment 400640 [details] work in progress.
Created attachment 400653 [details] proposed patch.
Comment on attachment 400653 [details] proposed patch. Ready for a review.
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 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 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 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 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 on attachment 400653 [details] proposed patch. Clearing r? for now
Created attachment 401990 [details] proposed patch.
I must be missing something. How does Options::useJIT() check that we have the JIT entitlement like VM::canUseJIT() does via VM::canUseAssembler()?
(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 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 on attachment 401990 [details] proposed patch. Taking out of review. Will look at a better abstraction for Options initialization before I propose another patch.
(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 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 on attachment 401990 [details] proposed patch. r=me Might be worth noting your clean up plan here for those not in the slack
(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.
Thanks for the reviews. Landed in r263117: <http://trac.webkit.org/r263117>.