The default bug view has changed. See this FAQ.

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

RESOLVED FIXED in mozilla9

Status

()

Core
XPConnect
P1
normal
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: bz, Assigned: bz)

Tracking

(Blocks: 1 bug)

Trunk
mozilla9
x86
Mac OS X
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(5 attachments)

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!
Created attachment 517616 [details]
Browser testcase
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.

Comment 4

6 years ago
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

6 years ago
> 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?

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

6 years ago
I updated my dropbox example with a workaround using script injection.
Assignee: general → jorendorff
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.
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?

/be
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).
tracking-firefox9: --- → ?
Oops, wrong bug.  Forgot the compileAndGo Function() issue was spun off from this, which is the issue that should get fixed.
tracking-firefox9: ? → ---
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
Created attachment 557329 [details] [diff] [review]
Get rid of the Window class setter so that SETNAME on the global is faster in the browser.
Attachment #557329 - Flags: review?(mrbkap)
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+
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
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.
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.
http://hg.mozilla.org/integration/mozilla-inbound/rev/ee8c8daffe43
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:

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

Example log: https://tbpl.mozilla.org/php/getParsedLog.php?id=6235807&full=1
Target Milestone: mozilla9 → ---
I was blameless, I say.

Relanded: http://hg.mozilla.org/integration/mozilla-inbound/rev/d6f8a08a4c85
Target Milestone: --- → mozilla9
http://hg.mozilla.org/mozilla-central/rev/d6f8a08a4c85
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED

Updated

6 years ago
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.