Closed Bug 639720 Opened 9 years ago Closed 9 years ago

JM: Different code generated in shell and browser (browser much slower) for global var access inside new Function


(Core :: XPConnect, defect, P1)






(Reporter: bzbarsky, Assigned: bzbarsky)




(5 files)

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


while for browser I see numbers like:


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:


while V8's numbers are:


So we have some headroom here!
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?
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.
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.

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).
> My Chrome 11.0.694.0 (canary build) test results are interesting. 
NM. Ignore that :P
> Do you still see a slowdown for JM in my reordered test?


> I tried FF4b2 but didn't see a problem

JM was added in b7, so you're just testing a very very old TM.
I updated my dropbox example with a workaround using script injection.
Assignee: general → jorendorff
Attached patch v1Splinter Review
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.
Attachment #517858 - Flags: feedback?(brendan)
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.
Yeah, indeed.  Why are we ending up in stubcall land at all, even for SETNAME?
Jason, should I give feedback or wait?

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.
Duplicate of this bug: 659941
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.
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...
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).
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).
Oops, wrong bug.  Forgot the compileAndGo Function() issue was spun off from this, which is the issue that should get fixed.
Brian, thanks for figuring out what's going on in the browser here!  Taking this bug; this should not be too terrible to fix.
Assignee: jorendorff → nobody
Component: JavaScript Engine → XPConnect
Priority: -- → P1
QA Contact: general → xpconnect
Assignee: nobody → bzbarsky
Whiteboard: [need review]
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.
Attachment #557329 - Flags: review?(mrbkap) → review+
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.
Attachment #557402 - Flags: review?(mrbkap)
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.
Attachment #557402 - Flags: review?(mrbkap) → review+
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?
> 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.
Flags: in-testsuite+
Whiteboard: [need review]
Target Milestone: --- → mozilla9
Backed out on the suspicion that this might be the culprit behind the Linux Debug red builds that we're seeing on TBPL:

Example log:
Target Milestone: mozilla9 → ---
I was blameless, I say.

Target Milestone: --- → mozilla9
Closed: 9 years ago
Resolution: --- → FIXED
Depends on: 684544
Attachment #517858 - Flags: feedback?(brendan)
Blocks: 660233
Depends on: 659350
Blocks: 517143
You need to log in before you can comment on or make changes to this bug.