Last Comment Bug 412985 - Provide stubs for JS_THREADSAFE APIs in non-JS_THREADSAFE builds
: Provide stubs for JS_THREADSAFE APIs in non-JS_THREADSAFE builds
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: All All
: -- normal (vote)
: mozilla1.9beta4
Assigned To: Jason Orendorff [:jorendorff]
:
Mentors:
Depends on:
Blocks:
  Show dependency treegraph
 
Reported: 2008-01-18 10:37 PST by Jason Orendorff [:jorendorff]
Modified: 2008-02-29 21:07 PST (History)
9 users (show)
bob: in‑testsuite-
bob: in‑litmus-
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v1 (3.81 KB, patch)
2008-02-03 00:06 PST, Jason Orendorff [:jorendorff]
brendan: review+
brendan: approval1.9+
Details | Diff | Review
v1 (2.64 KB, patch)
2008-02-12 20:37 PST, gyuyoung kim
no flags Details | Diff | Review
v2 (2.73 KB, patch)
2008-02-15 02:11 PST, gyuyoung kim
jorendorff: review+
brendan: superreview+
brendan: approval1.9+
Details | Diff | Review
v1 - related to nsDOMClassInfo.cpp file (2.29 KB, patch)
2008-02-24 09:21 PST, gyuyoung kim
no flags Details | Diff | Review

Description Jason Orendorff [:jorendorff] 2008-01-18 10:37:25 PST
Currently JS_BeginRequest and friends are #ifdef'd out in non-JS_THREADSAFE builds.  I think they should be no-ops instead.

Two use cases for this:

* To make it easier to share and reuse JSAPI application code across projects.  This might make life a little better for embedders that don't want JS_THREADSAFE.

* Guoxin Fan is interested in building the browser without JS_THREADSAFE, for an embedded device.  He needs the APIs stubbed out to get the browser to compile.  (My experience in ActionMonkey stage 0 indicates that this approach produces a working browser, but probably with crashes that will need to be tracked down and pondered.)
Comment 1 Guoxin Fan 2008-01-18 10:43:31 PST
My coworker Gyuyoung Kim and I try to find out whether there is meaningful gain in performances without JS_THREADSAFE in FireFox browser. 
Comment 2 gyuyoung kim 2008-01-18 23:19:59 PST
This is Gyuyoung Kim and Guoxin is my coworker.
BTW,why the target OS is selected as Mac OS? I'd like to start it on the Linux.
Comment 3 Jason Orendorff [:jorendorff] 2008-01-20 10:05:59 PST
My mistake.  Of course I meant "All".
Comment 4 gyuyoung kim 2008-01-21 02:40:04 PST
When we will do a regression test,I think we can refer to the "Mozilla automated testing" website.

URL: http://developer.mozilla.org/en/docs/Mozilla_automated_testing
Comment 5 timeless 2008-01-21 04:56:14 PST
If you want to use Gecko w/o JS_THREADSAFE, you should hack xpconnect so that it refuses to allow access to any js from any other thread, otherwise you will run into problems.

historically, there have been JavaScript components that ran on various threads, and we certainly allow for this. If you care about PAC and want non blocking behavior, the only way it can safely work is for it to run off the main thread, and it *is* javascript. Now, as it happens, PAC javascript doesn't technically need to be in the same engine as DOM, but the xpconnect we have today doesn't support multiple runtimes, and at the present time, PAC is hooked via xpconnect.

That said, I'd personally like to see this bug fixed, because then I could remove:
http://mxr.mozilla.org/mozilla/search?string=Request&find=jsdb&filter=define
Comment 6 Jason Orendorff [:jorendorff] 2008-02-03 00:06:11 PST
Created attachment 301087 [details] [diff] [review]
v1

Regardless of whether a non-JS_THREADSAFE browser is a sane thing to attempt, I think this is a good change.  Here's the patch.
Comment 7 Brendan Eich [:brendan] 2008-02-03 01:02:28 PST
For the Samsung guys, to help reading timeless's comment: PAC = Proxy AutoConfig.

http://mxr.mozilla.org/mozilla/search?string=Proxy+Auto+Config&find=&findi=&filter=&tree=mozilla

/be
Comment 8 Brendan Eich [:brendan] 2008-02-03 01:03:45 PST
Comment on attachment 301087 [details] [diff] [review]
v1

Again, safe for 1.9/fx3 and good to reduce merge workload.

/be
Comment 9 gyuyoung kim 2008-02-04 04:53:04 PST
Brendan and Jason,thank you for your advice and help. Recently, we had some internal work, so we couldn't concentrate upon this bug. But,we will try to solve this bug as soon as possible.

BTW,I tried to adjust the Jason's patch to firefox.(Firefox 3.0 beta 2 on Linux).
And then, when I removed the JS_THREADSAFE from a configure file,I met some compilation errors.  
 
For example, the nsScriptSecurityManager.cpp file needs the JS_GetClass() function with two parameters. but,if the JS_THREADSAFE is removed from a configure file, the jsapi.h provides JS_GetClass() with a parameter to outer module. So,I think we need additional work as well as  Jason's patch for fixing this bug. 

If I know wrong information, please give your input to me.
Comment 10 Jason Orendorff [:jorendorff] 2008-02-04 06:33:37 PST
Ah-- yes, that's true.  Some code that calls ::JS_GetClass or JS_GetClass would need to be changed to use the JS_GET_CLASS macro instead.  It looks like only 4 files are affected, though:

http://mxr.mozilla.org/seamonkey/search?string=JS_GetClass&find=&findi=&filter=\bJS_GetClass\b&tree=seamonkey

Calls that are inside js/src should not be changed, and some call sites are already ok, so that leaves:

  caps/src/nsSecurityManagerFactory.cpp
  dom/src/base/nsDOMClassInfo.h
  dom/src/base/nsJSEnvironment.cpp
  content/xbl/src/nsXBLBinding.cpp

What other errors are you seeing?
Comment 11 Brendan Eich [:brendan] 2008-02-04 11:38:11 PST
JS_GET_CLASS lacks elsewhere should be fodder for other bugs.

/be
Comment 12 gyuyoung kim 2008-02-04 23:19:01 PST
First, I found some error related to JS_GetClass().
 
  xpinstall/src/nsInstall.cpp
  xpinstall/src/nsJSInstall.cpp
  
And, XBLFinalize() of nsXBLBinding.cpp uses "static_cast", but JS_GET_CLASS can't be adapted it. 

Secondly, The other error is JSScopeProperty. After I replaced JS_GetClass() with JS_GET_CLASS  macro, the other compilation error was taken place in nsDOMClassInfo.cpp file.(Firefox3.0b2 on Linux)

The reason is that nsWindowSH::AddProperty() of nsDOMClassInfo.cpp file uses the JSScopeProperty, but it can't access JSScopeProperty. The error messages are as below.

==================================================================================
nsDOMClassInfo.cpp:4633: error: invalid use of undefined type ‘struct JSScopeProperty’
../../../dist/include/js/jsprvtd.h:119: error: forward declaration of ‘struct JSScopeProperty’
nsDOMClassInfo.cpp:4634: error: invalid use of undefined type ‘struct JSScopeProperty’
../../../dist/include/js/jsprvtd.h:119: error: forward declaration of ‘struct JSScopeProperty’
nsDOMClassInfo.cpp:4635: error: invalid use of undefined type ‘struct JSScopeProperty’
../../../dist/include/js/jsprvtd.h:119: error: forward declaration of ‘struct JSScopeProperty’
=====================================================================================

Comment 13 Brendan Eich [:brendan] 2008-02-04 23:38:19 PST
Really, these issues should go in a new bug or two.

About XBLFinalize: the line in question is

nsXBLJSClass* c = static_cast<nsXBLJSClass*>(::JS_GetClass(cx, obj));

and using JS_GET_CLASS instead should work fine -- the macro expands before C++ parses the static cast's parenthesized operand.

For the other problem, suggest you make nsDOMClassInfo.i and see where jsscope.h is included in a JS_THREADSAFE build, then see why that include is not occurring in your build.

/be
Comment 14 Reed Loden [:reed] (use needinfo?) 2008-02-06 13:34:02 PST
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.403; previous revision: 3.402
done
Checking in js/src/jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.181; previous revision: 3.180
done
Comment 15 gyuyoung kim 2008-02-12 20:37:42 PST
Created attachment 302978 [details] [diff] [review]
v1

Functions in the patch are called by xpconnect files. And currently,the JS_THREADSAFE is defined in makefiles under xpconnect. So,if the JS_THREADSAFE is removed in makefiles under xpconnect, compile errors will take place because of JS_THREADSAFE removal.
Comment 16 Jason Orendorff [:jorendorff] 2008-02-14 11:09:29 PST
Yep.  Sorry I missed these.  Thoughts on this patch:

* With the patch, the return values from these functions do not fulfill the promises made by the API.  (That is, the return values of all three functions do not reflect prior calls to Set and Clear, as the API says they should.)  I can't immediately tell whether that will break XPConnect.  (brendan, shaver?)

It's possible to implement the API:  add a field "uint8 fakeThreadId;" to JSContext when JS_THREADSAFE is not defined.  (This will fit in some unused padding somewhere.)  Initialize it to 1 in NewContext, have JS_SetContextThread(cx) assign cx->fakeThreadId=1, etc.  I don't know if this is necessary, though.

* In any case JS_SetContextThread shouldn't return -1.  0 is better.

* Nit: it looks like this patch leaves 3 blank lines in a row.  Please change it to one blank line.
Comment 17 gyuyoung kim 2008-02-15 02:11:08 PST
Created attachment 303500 [details] [diff] [review]
v2

I modified the patch by considering your comment.

In my opinion, we should modify three modules, Gecko, SpiderMonkey, and xpconnect for removing JS_THREADSAFE

SpiderMonkey: 
 - We are modifying jsapi.h/c files in SpiderMonkey.

xpconnect:
 - Currently, JS_THREADSAFE is defined in makefiles of xpconnect as a default option.
   I think the makefiles need to be modified as below, 

      E.g ) xpconnect/src/Makefile.in

      -DEFINES		+= -DJSFILE -DJS_THREADSAFE -DEXPORT_XPC_API
      
      +DEFINES		+= -DJSFILE -DEXPORT_XPC_API
      +
      +ifdef JS_THREADSAFE
      +DEFINES         += -DJS_THREADSAFE
      +endif

  But, I don't know why the JS_THREADSAFE is defined the makefile under xpconnect as a default option. As mentioned by timeless, xpconnect we have doesn't support multiple runtimes so that the JS_THREADSAFE is defined as a default option. If it is so, I think we need to implement the protection code for avoiding a runtime error when multiple thread accesses the SpiderMonkey, or leave it to use the JS_THREADSAFE in xpconnect.

Gecko:
   - As mentioned in the above, JS_GetClass should be changed into the JS_GET_CLASS in Gecko.

Now, I solved the above problems so as to make FF-3.0b2 up-and-running without the JS_THREADSAFE temporarily.
 And then, I tested it both with/without JS_THREADSAFE against SunSpider. FF without JS_THREADSAFE is about 10% better than the FF with JS_THREADSAFE. The results are as below.

 - With JS_THREADSAFE : http://webkit.org/perf/sunspider-0.9/sunspider-results.html?%7B%223d-cube%22:%5B834,737,721,719,720%5D,%223d-morph%22:%5B1400,1229,1244,1205,1296%5D,%223d-raytrace%22:%5B376,374,374,374,377%5D,%22access-binary-trees%22:%5B157,220,221,291,156%5D,%22access-fannkuch%22:%5B754,742,744,741,772%5D,%22access-nbody%22:%5B811,804,814,618,831%5D,%22access-nsieve%22:%5B271,270,268,339,274%5D,%22bitops-3bit-bits-in-byte%22:%5B353,351,363,357,356%5D,%22bitops-bits-in-byte%22:%5B394,350,352,351,452%5D,%22bitops-bitwise-and%22:%5B1195,1187,1102,1050,1230%5D,%22bitops-nsieve-bits%22:%5B393,395,409,295,296%5D,%22controlflow-recursive%22:%5B138,137,107,106,105%5D,%22crypto-aes%22:%5B403,403,389,382,380%5D,%22crypto-md5%22:%5B233,227,230,230,234%5D,%22crypto-sha1%22:%5B242,242,244,244,241%5D,%22date-format-tofte%22:%5B630,629,622,633,620%5D,%22date-format-xparb%22:%5B371,356,352,371,368%5D,%22math-cordic%22:%5B675,669,695,695,684%5D,%22math-partial-sums%22:%5B629,646,649,630,659%5D,%22math-spectral-norm%22:%5B232,316,311,312,309%5D,%22regexp-dna%22:%5B685,687,677,673,694%5D,%22string-base64%22:%5B568,566,569,572,570%5D,%22string-fasta%22:%5B619,624,620,617,619%5D,%22string-tagcloud%22:%5B492,467,489,490,495%5D,%22string-unpack-code%22:%5B915,925,917,915,919%5D,%22string-validate-input%22:%5B285,290,284,282,285%5D%7D

 - Without JS_THREADSAFE : http://webkit.org/perf/sunspider-0.9/sunspider-results.html?%7B%223d-cube%22:%5B650,637,645,637,635%5D,%223d-morph%22:%5B1226,1210,1218,981,1125%5D,%223d-raytrace%22:%5B425,433,434,331,342%5D,%22access-binary-trees%22:%5B195,190,181,163,132%5D,%22access-fannkuch%22:%5B714,715,702,704,705%5D,%22access-nbody%22:%5B764,748,789,750,759%5D,%22access-nsieve%22:%5B236,277,236,262,235%5D,%22bitops-3bit-bits-in-byte%22:%5B329,329,452,327,329%5D,%22bitops-bits-in-byte%22:%5B348,352,346,352,345%5D,%22bitops-bitwise-and%22:%5B941,1118,997,961,1036%5D,%22bitops-nsieve-bits%22:%5B361,368,359,357,268%5D,%22controlflow-recursive%22:%5B126,91,124,122,93%5D,%22crypto-aes%22:%5B448,263,332,375,342%5D,%22crypto-md5%22:%5B209,213,215,220,214%5D,%22crypto-sha1%22:%5B224,224,224,225,230%5D,%22date-format-tofte%22:%5B578,572,574,568,674%5D,%22date-format-xparb%22:%5B315,313,298,302,309%5D,%22math-cordic%22:%5B591,580,695,665,598%5D,%22math-partial-sums%22:%5B558,661,574,574,567%5D,%22math-spectral-norm%22:%5B289,290,216,282,284%5D,%22regexp-dna%22:%5B665,683,685,683,662%5D,%22string-base64%22:%5B447,436,457,454,453%5D,%22string-fasta%22:%5B499,502,501,500,506%5D,%22string-tagcloud%22:%5B432,416,439,409,414%5D,%22string-unpack-code%22:%5B798,788,788,795,805%5D,%22string-validate-input%22:%5B245,251,252,249,249%5D%7D

 Of course, we need to test it under various environments carefully.  In my opinion, at least, it shows the possibility of performance improvement.
Comment 18 Jason Orendorff [:jorendorff] 2008-02-15 05:15:58 PST
Comment on attachment 303500 [details] [diff] [review]
v2

Brendan, I guess that 10% difference is a regression.  Want a separate bug for that?
Comment 19 Jason Orendorff [:jorendorff] 2008-02-15 05:46:56 PST
Reopening.

Filed bug 417710 for JS_GetClass.
Comment 20 Brendan Eich [:brendan] 2008-02-15 17:23:08 PST
(In reply to comment #18)
> (From update of attachment 303500 [details] [diff] [review])
> Brendan, I guess that 10% difference is a regression.  Want a separate bug for
> that?

Absolutely, however, sayrer has been digging into js shell vs. browser SunSpider perf (I think -- hope it wasn't xpcshell, which does use JS_THREADSAFE) and has not seen thread safety costs pop up.

/be
Comment 21 Brendan Eich [:brendan] 2008-02-15 17:24:48 PST
Comment on attachment 303500 [details] [diff] [review]
v2

No sr for SpiderMonkey, could use addl. review but you are good to go with Jason's review.

/be
Comment 22 Robert Sayre 2008-02-15 17:27:05 PST
(In reply to comment #20)
> 
> Absolutely, however, sayrer has been digging into js shell vs. browser
> SunSpider perf (I think -- hope it wasn't xpcshell, which does use
> JS_THREADSAFE) and has not seen thread safety costs pop up.

xpcshell is JS_THREADSAFE, aiui. The interesting comparison would be js shell vs JS_THREADSAFE js shell, something I haven't done.
Comment 23 Brendan Eich [:brendan] 2008-02-15 17:47:38 PST
Indeed, so Jason: please do file that bug. Thanks,

/be
Comment 24 Jason Orendorff [:jorendorff] 2008-02-19 09:53:36 PST
Filed bug 418436.
Comment 25 Reed Loden [:reed] (use needinfo?) 2008-02-20 03:22:25 PST
Checking in js/src/jsapi.c;
/cvsroot/mozilla/js/src/jsapi.c,v  <--  jsapi.c
new revision: 3.420; previous revision: 3.419
done
Checking in js/src/jsapi.h;
/cvsroot/mozilla/js/src/jsapi.h,v  <--  jsapi.h
new revision: 3.185; previous revision: 3.184
done
Comment 26 gyuyoung kim 2008-02-24 09:21:36 PST
Created attachment 305370 [details] [diff] [review]
v1 - related to nsDOMClassInfo.cpp file 

I found a reason of compilation error on nsDOMClassInfo.cpp. The nsDOMClassInfo.cpp file includes some SpiderMonkey files as below, (I made this patch with FF 3.0 beta 3)

 //JavaScript includes
   #include "jsapi.h"
   #include "jsnum.h"
   #include "jsdbgapi.h"
   #include "jscntxt.h"

And, jsscope.h file is included through “jscntxt.h -> jsatom.h -> jslock.h -> jsscope.h” into nsDOMClassInfo.cpp. But, the jsatom.h file only includes the jslock.h when JS_THREADSAFE is turned on as below. 

  54: #ifdef JS_THREADSAFE
  55: #include "jslock.h"
  56: #endif

And also, above source code of jsatom.h influences some files in xpconnect module when I turned off the JS_THREADSAFE
-	Js/src/xpconnect/src/xpcinline.h (This file can’t access ID_TO_VALUE macro in jsscope.h)
-	Js/src/xpconnect/src/xpcjsruntime.cpp (This file can’t access JS_LOCK_GC and JS_UNLOCK_GC macro in jslock.h)

Furthermore, the jslock.h file only includes the jsscope.h file when JS_THREADSAFE is turned on as below,

   42: #ifdef JS_THREADSAFE
   ...
   115: JS_END_EXTERN_C
   116: #include "jsscope.h"
   117: JS_BEGIN_EXTERN_C

To solve the compilation errors, I modified jslock.h and jsatom.h like the patch
 
I would like to get an advice about the patch.
Comment 27 Jason Orendorff [:jorendorff] 2008-02-28 07:05:40 PST
Please file a new bug for this new change.  This bug is resolved.

The patch looks like a good change.  Here are my comments:

In addition to moving the '#include "jsscope.h"' in jslock.h, also move the preceding comment preaching about how headers shouldn't be conditionally included--it'll make even more sense now.

Please also go ahead and delete the blank line after '#include "jspubtd.h"' in jsatom.h.
Comment 28 gyuyoung kim 2008-02-28 08:13:11 PST
I will make a new bug related to this problem. 

BTW, I'd like to make a new bug related to xpconnect as well.
As mentioned in comment 17, xpconnect can only be compiled with JS_THREADSAFE.

So,I think the xpconnect also can be built both with and without JS_THREASAFE.
Comment 29 Brendan Eich [:brendan] 2008-02-29 21:07:51 PST
Comment on attachment 305370 [details] [diff] [review]
v1 - related to nsDOMClassInfo.cpp file 

In a new bug, please.

/be

Note You need to log in before you can comment on or make changes to this bug.