Last Comment Bug 639720 - JM: Different code generated in shell and browser (browser much slower) for global var access inside new Function
: JM: Different code generated in shell and browser (browser much slower) for g...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: XPConnect (show other bugs)
: Trunk
: x86 Mac OS X
: P1 normal (vote)
: mozilla9
Assigned To: Boris Zbarsky [:bz]
:
Mentors:
Depends on: 659350 684544
Blocks: 517143 660233
  Show dependency treegraph
 
Reported: 2011-03-07 18:32 PST by Boris Zbarsky [:bz]
Modified: 2011-11-26 11:51 PST (History)
16 users (show)
bzbarsky: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Shell testcase (640 bytes, text/plain)
2011-03-07 18:32 PST, Boris Zbarsky [:bz]
no flags Details
Browser testcase (221 bytes, text/html)
2011-03-07 18:32 PST, Boris Zbarsky [:bz]
no flags Details
v1 (5.33 KB, patch)
2011-03-08 13:51 PST, Jason Orendorff [:jorendorff]
no flags Details | Diff | Splinter Review
Get rid of the Window class setter so that SETNAME on the global is faster in the browser. (11.96 KB, patch)
2011-08-31 14:33 PDT, Boris Zbarsky [:bz]
mrbkap: review+
Details | Diff | Splinter Review
Interdiff to fix test failures (2.59 KB, patch)
2011-08-31 20:40 PDT, Boris Zbarsky [:bz]
mrbkap: review+
Details | Diff | Splinter Review

Description Boris Zbarsky [:bz] 2011-03-07 18:32:23 PST
Created attachment 517615 [details]
Shell testcase

See the attached testcase, both shell and and browser versions.  For the shell version, I see numbers like:

804
685
761
595

while for browser I see numbers like:

945
688
1903
582

Notice that third number.

This is biting us on jsperf tests, because they use |new Function| for their testcases, and it's really easy to write testcases that then poke global vars from inside the created function (in fact you have to try hard to avoid it).

I profiled both the shell and browser version of that third test, and while shell is all in mjit-generated code in browser we end up calling stubs::SetName which calls js_NativeSet.  I don't know why the difference, but the time under SetName is approximately the time delta between the browser and shell.

Oh, in case we care, the TM numbers for this test are:

216
217
221
194

while V8's numbers are:

1080
265
266
128

So we have some headroom here!
Comment 1 Boris Zbarsky [:bz] 2011-03-07 18:32:59 PST
Created attachment 517616 [details]
Browser testcase
Comment 2 Boris Zbarsky [:bz] 2011-03-07 18:35:12 PST
In case that matters, f3 in the testcase compiles to using getxprop for "fun3".  I bet if we made it compile to getglobal it would all Just Work, but I don't understand why the getxprop is slower in browser?  Is it because there's a class getter hook on the Window or something?
Comment 3 Boris Zbarsky [:bz] 2011-03-07 18:59:25 PST
Oh, I guess the issue is more set than get....  In that case this is the difference between the setname in fun3 and the setgname in fun2, say.
Comment 4 John-David Dalton 2011-03-07 23:27:44 PST
My Chrome 11.0.694.0 (canary build) test results are interesting. I modified the test page to switch test order and it seems the first one is still the slowest (regardless of which test it is). Could be Crankshaft kicking in.

http://dl.dropbox.com/u/513327/moz_bug639720.html

Do you still see a slowdown for JM in my reordered test?
I tried FF4b2 but didn't see a problem (I bet I need a newer FF4).
Comment 5 John-David Dalton 2011-03-07 23:30:30 PST
> My Chrome 11.0.694.0 (canary build) test results are interesting. 
NM. Ignore that :P
Comment 6 Boris Zbarsky [:bz] 2011-03-08 06:50:07 PST
> Do you still see a slowdown for JM in my reordered test?

Yes.

> I tried FF4b2 but didn't see a problem

JM was added in b7, so you're just testing a very very old TM.
Comment 7 John-David Dalton 2011-03-08 10:38:37 PST
I updated my dropbox example with a workaround using script injection.
Comment 8 Jason Orendorff [:jorendorff] 2011-03-08 13:51:32 PST
Created attachment 517858 [details] [diff] [review]
v1

I don't really know what I'm doing here...

This patch turns on COMPILE_N_GO compilation for Function only. I left it off for JS_CompileFunction because COMPILE_N_GO breaks XDR. There's at least a jsapi-test using JS_CompileFunction to create functions and then clone them across compartments, which sends them through XDR.

The Right Way might be to reimplement Compiler::compileFunctionBody to build a function declaration and compile that -- more work than I can take on right now.
Comment 9 Jason Orendorff [:jorendorff] 2011-03-08 13:51:51 PST
Now that I'm thinking about it, it's pretty disturbing that SETNAME is that much slower than SETGNAME. The global property exists, it has the default setter... this should be fast. I want to look into that.
Comment 10 Boris Zbarsky [:bz] 2011-03-08 14:01:39 PST
Yeah, indeed.  Why are we ending up in stubcall land at all, even for SETNAME?
Comment 11 Brendan Eich [:brendan] 2011-05-11 10:51:17 PDT
Jason, should I give feedback or wait?

/be
Comment 12 Boris Zbarsky [:bz] 2011-05-26 08:58:08 PDT
And I still don't understand why there's a difference between browser and shell, btw...  both should be using SETNAME as things stand, I'd think.
Comment 13 Brian Hackett (:bhackett) 2011-05-26 09:01:30 PDT
*** Bug 659941 has been marked as a duplicate of this bug. ***
Comment 14 Brian Hackett (:bhackett) 2011-05-26 09:06:15 PDT
The name IC needs to walk and cache the scope chains, which probably look different between the browser and shell.  If JM sees something it doesn't like in the browser scope chain it will disable the cache and use a stub.  That needs investigation, but seems like it should be a new bug.
Comment 15 Boris Zbarsky [:bz] 2011-05-26 09:33:07 PDT
I agree we want separate bugs for that problem and the compile-n-go Function thing.

I'd be tempted to use bug 659941 for the latter and this bug for the former, since the former problem is what this bug is filed on originally...
Comment 16 Brian Hackett (:bhackett) 2011-05-26 09:36:55 PDT
Ah, makes sense (was just looking at the patch, which will produce the fastest code but doesn't address the root problem in the mjit).
Comment 17 Brian Hackett (:bhackett) 2011-08-31 07:39:44 PDT
Requesting tracking for Fx9.  With TI landed the compiler is much more sensitive to whether code is compileAndGo --- global accesses have sped up, generic name accesses have slowed down some, and type information is much more precise in compileAndGo scripts.  This should be a simple fix.

Also, bug 662704 comment 8 explains the SETNAME browser-only regression: Window objects have a class setter hook which disables IC generation for SETNAME ops which hit the global object in the browser (but not the shell).
Comment 18 Brian Hackett (:bhackett) 2011-08-31 07:43:07 PDT
Oops, wrong bug.  Forgot the compileAndGo Function() issue was spun off from this, which is the issue that should get fixed.
Comment 19 Boris Zbarsky [:bz] 2011-08-31 14:15:46 PDT
Brian, thanks for figuring out what's going on in the browser here!  Taking this bug; this should not be too terrible to fix.
Comment 20 Boris Zbarsky [:bz] 2011-08-31 14:33:36 PDT
Created attachment 557329 [details] [diff] [review]
Get rid of the Window class setter so that SETNAME on the global is faster in the browser.
Comment 21 Blake Kaplan (:mrbkap) 2011-08-31 14:42:02 PDT
Comment on attachment 557329 [details] [diff] [review]
Get rid of the Window class setter so that SETNAME on the global is faster in the browser.

Review of attachment 557329 [details] [diff] [review]:
-----------------------------------------------------------------

::: dom/base/nsDOMClassInfo.cpp
@@ +6379,5 @@
> +
> +  rv = xpcomObj->GetLocation(getter_AddRefs(location));
> +  NS_ENSURE_SUCCESS(rv, rv);
> +
> +  JSAutoRequest ar(cx);

No need for this JSAutoRequest.
Comment 22 Ed Morley [:emorley] 2011-08-31 17:05:06 PDT
Something in this push caused orange, so backed out of inbound:
https://tbpl.mozilla.org/?tree=Mozilla-Inbound&usebuildbot=1&rev=89b87e96dc17
http://hg.mozilla.org/integration/mozilla-inbound/rev/89b87e96dc17
Comment 23 Boris Zbarsky [:bz] 2011-08-31 20:40:44 PDT
Created attachment 557402 [details] [diff] [review]
Interdiff to fix test failures

There were two sources of test failures:

1) We were stomping on some already-pending security exceptions in some cases.
2) We were not setting *vp to the Location object, which caused the string passed to the setter
   to be stored in the slot on the inner window.  Then the following .location get would get
   the string, not the Location object.  Fixed that by restoring the code for setting *vp.
Comment 24 Blake Kaplan (:mrbkap) 2011-09-01 01:27:17 PDT
Comment on attachment 557402 [details] [diff] [review]
Interdiff to fix test failures

I wonder if XPConnect should be the one to not stomp over exceptions in this case, I bet we already have a bug on file about that. I'd forgotten that we actually hold the location in the slot, sorry about that.
Comment 25 :Ms2ger (⌚ UTC+1/+2) 2011-09-01 06:17:26 PDT
Comment on attachment 557329 [details] [diff] [review]
Get rid of the Window class setter so that SETNAME on the global is faster in the browser.

>--- a/dom/base/nsDOMClassInfo.cpp
>+++ b/dom/base/nsDOMClassInfo.cpp
>+LocationSetterGuts(JSContext *cx, JSObject *obj, jsval *vp)
>+{
>+  nsresult rv = NS_OK;
>+  nsCOMPtr<nsIDOMLocation> location;

Could you move these two declarations closer to their use?
Comment 26 Boris Zbarsky [:bz] 2011-09-01 12:40:13 PDT
> I wonder if XPConnect should be the one to not stomp over exceptions in this case

XPConnect is not involved at all; this is pure manual JSAPI munging going on here and in nsDOMClassInfo::ThrowJSException.

> Could you move these two declarations closer to their use?

Done; good catch.  In an earlier patch iteration they made more sense where they are now.
Comment 28 :Ehsan Akhgari (away Aug 1-5) 2011-09-01 18:05:05 PDT
Backed out on the suspicion that this might be the culprit behind the Linux Debug red builds that we're seeing on TBPL:

http://hg.mozilla.org/integration/mozilla-inbound/rev/dc27139edda0

Example log: https://tbpl.mozilla.org/php/getParsedLog.php?id=6235807&full=1
Comment 29 Boris Zbarsky [:bz] 2011-09-02 10:47:00 PDT
I was blameless, I say.

Relanded: http://hg.mozilla.org/integration/mozilla-inbound/rev/d6f8a08a4c85
Comment 30 Marco Bonardo [::mak] (Away 6-20 Aug) 2011-09-03 02:56:59 PDT
http://hg.mozilla.org/mozilla-central/rev/d6f8a08a4c85

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