Bug 34855 – Add a flag to enable JSC JIT in Android, disabled by default

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

[Harry Wu]

Add code that enables SquirrelFish Extreme (a.k.a JSCX, JSC JIT) in Android.
It's disabled by default, but can be enabled with a enveronment variable.

[Harry Wu]

I've already got the code working, please assign it to myself.

[Harry Wu]

Created attachment 48916 [details]
The patch that adds a flag to enable JSC JIT in Android.

[WebKit Review Bot]

Attachment 48916 [details] did not pass style-queue:

Failed to run "WebKitTools/Scripts/check-webkit-style" exit_code: 1
JavaScriptCore/ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
JavaScriptCore/ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:5:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:6:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:7:  Line contains tab character.  [whitespace/tab] [5]
ChangeLog:8:  Line contains tab character.  [whitespace/tab] [5]
Total errors found: 8


If any of these errors are false positives, please file a bug against check-webkit-style.

[Ariya Hidayat]

Comment on attachment 48916 [details]
The patch that adds a flag to enable JSC JIT in Android.

The patch looks fine, but please fix the tabs in the ChangeLog (like the style bot says).

[Harry Wu]

Created attachment 49013 [details]
The revised patch that removes the style violations.

[Harry Wu]

(In reply to comment #4)
> (From update of attachment 48916 [details])
> The patch looks fine, but please fix the tabs in the ChangeLog (like the style
> bot says).
Hello, I've updated the patch, could you have another look? Thanks!

[Laszlo Gombos]

Comment on attachment 49013 [details]
The revised patch that removes the style violations.

> Index: Android.mk
> ===================================================================
> --- Android.mk	(revision 54822)
> +++ Android.mk	(working copy)
> @@ -33,6 +33,8 @@ LOCAL_PATH := $(call my-dir)
>  #    To help setup buildbot, a new environment variable, USE_ALT_JS_ENGINE,
>  #    can be set to true, so that two builds can be different but without
>  #    specifying which JS engine to use.
> +# To enable JIT in Android's JSC, please set ENABLE_ANDROID_JSC_JIT environment
> +# variable to true.
>  
>  # Read JS_ENGINE environment variable
>  JAVASCRIPT_ENGINE = $(JS_ENGINE)
> @@ -199,6 +201,14 @@ LOCAL_CFLAGS += -fno-strict-aliasing
>  LOCAL_CFLAGS += -include "WebCorePrefix.h"
>  LOCAL_CFLAGS += -fvisibility=hidden
>  
> +# Enable JSC JIT if JSC is used and ENABLE_ANDROID_JSC_JIT environment
> +# variable is set to true
> +ifeq ($(JAVASCRIPT_ENGINE),jsc)
> +ifeq ($(ENABLE_ANDROID_JSC_JIT),true)

The name of the environment variable does not seems to be consistent with the existing environment configuration option for Android. I would suggest to drop the ANDROID name from the name of the environment variable and change ENABLE_ANDROID_JSC_JIT to ENABLE_JSC_JIT.

> +LOCAL_CFLAGS += -DENABLE_ANDROID_JSC_JIT=1

Instead of introducing a new (ENABLE_ANDROID_JSC_JIT) C macro that WebKit source tree needs to be aware of, can we maybe use the the existing defines so that Platform.h does not need to change ? Some other ports are using the same technique, see for example JavaScriptCore.pri 

LOCAL_CFLAGS += "-DENABLE_JIT=1 -DENABLE_YARR=1 -DENABLE_YARR_JIT=1"

> +endif
> +endif
> +

The proposed patch is correct, but I'd like to see the inconsistencies with the existing code addressed.

[Harry Wu]

Created attachment 49223 [details]
Revised patch that changes ENABLE_ANDROID_JSC_JIT environment variable to ENABLE_JSC_JIT.

[Harry Wu]

(In reply to comment #7)
> (From update of attachment 49013 [details])
> > Index: Android.mk
> > ===================================================================
> > --- Android.mk	(revision 54822)
> > +++ Android.mk	(working copy)
> > @@ -33,6 +33,8 @@ LOCAL_PATH := $(call my-dir)
> >  #    To help setup buildbot, a new environment variable, USE_ALT_JS_ENGINE,
> >  #    can be set to true, so that two builds can be different but without
> >  #    specifying which JS engine to use.
> > +# To enable JIT in Android's JSC, please set ENABLE_ANDROID_JSC_JIT environment
> > +# variable to true.
> >  
> >  # Read JS_ENGINE environment variable
> >  JAVASCRIPT_ENGINE = $(JS_ENGINE)
> > @@ -199,6 +201,14 @@ LOCAL_CFLAGS += -fno-strict-aliasing
> >  LOCAL_CFLAGS += -include "WebCorePrefix.h"
> >  LOCAL_CFLAGS += -fvisibility=hidden
> >  
> > +# Enable JSC JIT if JSC is used and ENABLE_ANDROID_JSC_JIT environment
> > +# variable is set to true
> > +ifeq ($(JAVASCRIPT_ENGINE),jsc)
> > +ifeq ($(ENABLE_ANDROID_JSC_JIT),true)
> 
> The name of the environment variable does not seems to be consistent with the
> existing environment configuration option for Android. I would suggest to drop
> the ANDROID name from the name of the environment variable and change
> ENABLE_ANDROID_JSC_JIT to ENABLE_JSC_JIT.
Updated.

> 
> > +LOCAL_CFLAGS += -DENABLE_ANDROID_JSC_JIT=1
> 
> Instead of introducing a new (ENABLE_ANDROID_JSC_JIT) C macro that WebKit
> source tree needs to be aware of, can we maybe use the the existing defines so
> that Platform.h does not need to change ? Some other ports are using the same
> technique, see for example JavaScriptCore.pri 
> 
> LOCAL_CFLAGS += "-DENABLE_JIT=1 -DENABLE_YARR=1 -DENABLE_YARR_JIT=1"
I think adding a new C macro makes more sense for following reasons:
1. The new code is consistent with the style of the code that enables JIT on iPhone and other platform. A general webkit reader can quickly understand which platforms JIT is working well.
2. There are cases, that Android is not running on an ARM-Thumb2 core, and the proposed code with new C macro is able to detect that and override the flag.
3. We (Android team) is measuring the performance for JIT, if we decide to use JIT as default later, we'll need to touch Platform.h anyway. 

> 
> > +endif
> > +endif
> > +
> 
> The proposed patch is correct, but I'd like to see the inconsistencies with the
> existing code addressed.

[Eric Seidel (no email)]

Comment on attachment 49223 [details]
Revised patch that changes ENABLE_ANDROID_JSC_JIT environment variable to ENABLE_JSC_JIT.

OK.

[WebKit Commit Bot]

Comment on attachment 49223 [details]
Revised patch that changes ENABLE_ANDROID_JSC_JIT environment variable to ENABLE_JSC_JIT.

Clearing flags on attachment: 49223

Committed r55117: <http://trac.webkit.org/changeset/55117>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.