Bug 218201 – [WPE][GTK] Ensure either ENABLE_JIT or ENABLE_C_LOOP is enabled(?)

• Status: RESOLVED
• Resolution: WONTFIX
• Component: WebKitGTK

[Michael Catanzaro]

In bug #217989, I mentioned that Red Hat and SUSE accidentally shipped WebKit builds with both JIT and cloop disabled. i.e. JSC was using assembler-based llint interpreter rather than cloop. Carlos Lopez requested we add a check to prevent this from happening:

(In reply to Carlos Alberto Lopez Perez from comment #12)
> I think we should add a check in cmake to ensure that one (and only one) of
> JIT or C_LOOP is enabled, so that we don't allow building with both options
> disabled (or with both enabled)

That makes sense to me. I would do this only in OptionsGTK.cmake and OptionsWPE.cmake, though, because I know Apple does not want that restriction for its ports, and we don't want to make transitioning to CMake any harder. But because ENABLE_C_LOOP and ENABLE_JIT are both PRIVATE settings, and WebKitFeatures.cmake normally ensures exactly one or the other is always enabled, checking to ensure it hasn't somehow happened probably makes sense for us. We could change that and remove this restriction if Igalia JSC developers ever want to do so, but that's the status quo currently, and enforcing it would avoid mistakes where we accidentally ship with a different JSC than everybody else, with our own special bugs.

Counterargument: we don't support changing the values of PRIVATE settings, and this error will never happen if you don't do that. If bug #217989 is fixed, then distros will no longer have to mess with PRIVATE settings, so there is less value in adding that check. Alternative: maybe you really do know what you're doing and intentionally want to experiment with both JIT and cloop disabled? If so, then removing this check manually will be annoying.

Anyway, I'll submit a patch, and we can see what people think.

[Michael Catanzaro]

The more I think about this, the more I kinda don't want to do it. If you mess with private options, you should know what you're doing, right? And I imagine this configuration could be useful for JSC developers?

Anyway, no strong opinion from me. Patch is incoming regardless.

[Michael Catanzaro]

Created attachment 412350 [details]
Patch

[Carlos Alberto Lopez Perez]

(In reply to Michael Catanzaro from comment #0)
> In bug #217989, I mentioned that Red Hat and SUSE accidentally shipped
> WebKit builds with both JIT and cloop disabled. i.e. JSC was using
> assembler-based llint interpreter rather than cloop. 

I'm curious.. 

Does JSC work fine with this combination (both JIT and C_LOOP disabled) ? I thought this was not supported, but maybe it is?

[Michael Catanzaro]

(In reply to Carlos Alberto Lopez Perez from comment #3)
> I'm curious.. 
> 
> Does JSC work fine with this combination (both JIT and C_LOOP disabled) ? I
> thought this was not supported, but maybe it is?

I think it works fine? It ought to work, but I'm not certain if it actually does.

It's "not supported" at the GTK/WPE public build options level. It *is* supported at the JSC level. (JSC devs would know way more about this than me, but I'm pretty sure Apple ships this configuration on some device. Maybe watchOS?) That's why I kinda think we should not commit this. As long as we don't *have* to use private options to make WebKit work (as we currently must, bug #217989), then people probably won't use the private options unless they know what they're doing... hopefully.

[Xan Lopez]

Just to clarify, since I feel there could be a slight confusion here: CLOOP is another variant of LLInt. The C++, portable one, instead of the native implementations (x86_64, various flavors of ARM, MIPS, etc). Logically it sits at the same level than LLInt in this conversation.

That being said...

CLOOP and JIT disabled is a reasonable combination that is used by people in the real world, and is used when developing and debugging the engine itself (although in that case we typically disable tiers at runtime). It just means you only have the interpreter available and you are using the native implementations instead of CLOOP, the C++/portable one.

CLOOP and JIT enabled, as far as I know, will not work, because there's an assumption in JSC about LLInt/baseline JIT sharing stack representations, among other things. In practice I also don't know who would bother making the baseline JIT work but leave LLInt with only the CLOOP variant.

So it *could* make sense to prevent the previous combination to be enabled, but not sure this makes much sense. The way I see it distros should just get good defaults for their architectures, and not mess too much with these things because they are unlikely to get it right unless they have a pretty decent understanding of JSC, which seems overkill for a distributor.

[Guillaume Emont]

(In reply to Xan Lopez from comment #5)
> Just to clarify, since I feel there could be a slight confusion here: CLOOP
> is another variant of LLInt. The C++, portable one, instead of the native
> implementations (x86_64, various flavors of ARM, MIPS, etc). Logically it
> sits at the same level than LLInt in this conversation.
> 
> That being said...
> 
> CLOOP and JIT disabled is a reasonable combination that is used by people in
> the real world, and is used when developing and debugging the engine itself
> (although in that case we typically disable tiers at runtime). It just means
> you only have the interpreter available and you are using the native
> implementations instead of CLOOP, the C++/portable one.

To add to that point: this configuration (LLInt with asm backend, no JIT) is also likely to be the fastest combination available for environment where a JIT is not possible (e.g. due to a security policy not allowing JITs, I think that might be the case on the Playstation for instance).

> 
> So it *could* make sense to prevent the previous combination to be enabled,
> but not sure this makes much sense. The way I see it distros should just get
> good defaults for their architectures, and not mess too much with these
> things because they are unlikely to get it right unless they have a pretty
> decent understanding of JSC, which seems overkill for a distributor.

+1 to that. I am also thinking that this issue could be better addressed by providing better documentation on the subject, which hopefully would avoid confusion in the future.

[Guillaume Emont]

Comment on attachment 412350 [details]
Patch

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

Not a reviewer, but I disagree with this change, as it's useful to allow this configuration, as explained in other comments.

> ChangeLog:13
> +        different JSC than everybody else, with special codepaths and special bugs.

There aren't much specific codepaths that asm LLInt+no JIT would take, compared to when the JIT is enabled. All the code goes through the LLInt before being JIT'ed, so this code path is well tested. Also, EWS and buildbots run tests with `--useJIT=false` (and LLInt with asm backend), which has very similar codepaths to the case you want to forbid.

Are there specific bugs you encountered that happened only in this configuration?

[Michael Catanzaro]

(In reply to Guillaume Emont from comment #7)
> Not a reviewer, but I disagree with this change, as it's useful to allow
> this configuration, as explained in other comments.

OK, I'll close this then.

(In reply to Guillaume Emont from comment #7)
> Are there specific bugs you encountered that happened only in this
> configuration?

Nope. Either it works fine, or nobody has tested it and complained that it doesn't. ;)

[Michael Catanzaro]

(In reply to Xan Lopez from comment #5)
> The way I see it distros should just get
> good defaults for their architectures, and not mess too much with these
> things because they are unlikely to get it right unless they have a pretty
> decent understanding of JSC, which seems overkill for a distributor.

Currently we do the right thing for all architectures except aarch64, bug #217989. Any comment in that issue would be great.