3.81 KB, patch
|Details | Diff | Splinter Review|
2.73 KB, patch
|Details | Diff | Splinter Review|
2.29 KB, patch
|Details | Diff | Splinter Review|
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.)
My coworker Gyuyoung Kim and I try to find out whether there is meaningful gain in performances without JS_THREADSAFE in FireFox browser.
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.
My mistake. Of course I meant "All".
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
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.
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 on attachment 301087 [details] [diff] [review] v1 Again, safe for 1.9/fx3 and good to reduce merge workload. /be
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.
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?
JS_GET_CLASS lacks elsewhere should be fodder for other bugs. /be
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’ =====================================================================================
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
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
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.
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 on attachment 303500 [details] [diff] [review] v2 Brendan, I guess that 10% difference is a regression. Want a separate bug for that?
Reopening. Filed bug 417710 for JS_GetClass.
(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 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
(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.
Indeed, so Jason: please do file that bug. Thanks, /be
Filed bug 418436.
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
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.
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 on attachment 305370 [details] [diff] [review] v1 - related to nsDOMClassInfo.cpp file In a new bug, please. /be