Bug 34855 - Add a flag to enable JSC JIT in Android, disabled by default
Summary: Add a flag to enable JSC JIT in Android, disabled by default
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Android Android
: P2 Normal
Assignee: Harry Wu
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2010-02-11 11:27 PST by Harry Wu
Modified: 2010-02-22 17:55 PST (History)
4 users (show)

See Also:


Attachments
The patch that adds a flag to enable JSC JIT in Android. (4.14 KB, patch)
2010-02-17 11:10 PST, Harry Wu
ariya.hidayat: review-
Details | Formatted Diff | Diff
The revised patch that removes the style violations. (4.19 KB, patch)
2010-02-18 08:06 PST, Harry Wu
no flags Details | Formatted Diff | Diff
Revised patch that changes ENABLE_ANDROID_JSC_JIT environment variable to ENABLE_JSC_JIT. (4.15 KB, patch)
2010-02-22 10:15 PST, Harry Wu
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Harry Wu 2010-02-11 11:27:51 PST
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.
Comment 1 Harry Wu 2010-02-11 11:32:02 PST
I've already got the code working, please assign it to myself.
Comment 2 Harry Wu 2010-02-17 11:10:18 PST
Created attachment 48916 [details]
The patch that adds a flag to enable JSC JIT in Android.
Comment 3 WebKit Review Bot 2010-02-17 11:12:51 PST
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.
Comment 4 Ariya Hidayat 2010-02-17 11:50:19 PST
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).
Comment 5 Harry Wu 2010-02-18 08:06:49 PST
Created attachment 49013 [details]
The revised patch that removes the style violations.
Comment 6 Harry Wu 2010-02-19 07:48:14 PST
(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!
Comment 7 Laszlo Gombos 2010-02-20 05:03:02 PST
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.
Comment 8 Harry Wu 2010-02-22 10:15:07 PST
Created attachment 49223 [details]
Revised patch that changes ENABLE_ANDROID_JSC_JIT environment variable to ENABLE_JSC_JIT.
Comment 9 Harry Wu 2010-02-22 10:23:41 PST
(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.
Comment 10 Eric Seidel (no email) 2010-02-22 13:38:11 PST
Comment on attachment 49223 [details]
Revised patch that changes ENABLE_ANDROID_JSC_JIT environment variable to ENABLE_JSC_JIT.

OK.
Comment 11 WebKit Commit Bot 2010-02-22 17:55:44 PST
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>
Comment 12 WebKit Commit Bot 2010-02-22 17:55:49 PST
All reviewed patches have been landed.  Closing bug.