Boris Zbarsky [:bzbarsky] Assignee
	Description • 13 years ago

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!

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 2 • 13 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 3 • 13 years ago

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.

	John-David Dalton
	Comment 4 • 13 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).

	John-David Dalton
	Comment 5 • 13 years ago

> My Chrome 11.0.694.0 (canary build) test results are interesting. 
NM. Ignore that :P

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 6 • 13 years ago

> 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.

	John-David Dalton
	Comment 7 • 13 years ago

I updated my dropbox example with a workaround using script injection.

	Jason Orendorff [:jorendorff]
	Comment 8 • 13 years ago

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.

	Jason Orendorff [:jorendorff]
	Comment 9 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 10 • 13 years ago

Yeah, indeed.  Why are we ending up in stubcall land at all, even for SETNAME?

	Brendan Eich [:brendan]
	Comment 11 • 13 years ago

Jason, should I give feedback or wait?

/be

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 12 • 13 years ago

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.

	Brian Hackett [Laid off!]
	Comment 14 • 13 years ago

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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 15 • 13 years ago

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...

	Brian Hackett [Laid off!]
	Comment 16 • 13 years ago

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).

	Brian Hackett [Laid off!]
	Comment 17 • 13 years ago

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).

	Brian Hackett [Laid off!]
	Comment 18 • 13 years ago

Oops, wrong bug.  Forgot the compileAndGo Function() issue was spun off from this, which is the issue that should get fixed.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 19 • 13 years ago

Brian, thanks for figuring out what's going on in the browser here!  Taking this bug; this should not be too terrible to fix.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 21 • 13 years ago

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.

	Ed Morley [:emorley]
	Comment 22 • 13 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 23 • 13 years ago

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.

	Blake Kaplan (:mrbkap) (inactive)
	Comment 24 • 13 years ago

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.

	:Ms2ger (he/him; ⌚ UTC+1/+2)
	Comment 25 • 13 years ago

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?

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 26 • 13 years ago

> 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.

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 27 • 13 years ago

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

	(no longer active)
	Comment 28 • 13 years ago

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

	Boris Zbarsky [:bzbarsky] Assignee
	Comment 29 • 13 years ago

I was blameless, I say.

Relanded: http://hg.mozilla.org/integration/mozilla-inbound/rev/d6f8a08a4c85

	Marco Bonardo [:mak] (Away Apr 25 - May 5)
	Comment 30 • 13 years ago

http://hg.mozilla.org/mozilla-central/rev/d6f8a08a4c85