481514 - PurgeScopeChain should not deep-bail quite so eagerly

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Jason Orendorff [:jorendorff]]

The comment I added in bug 480856 contains a glaring misunderstanding.  The global shape will not necessarily change.  We should deep-bail only if it does, of course!

(Feel free to take this but I'll get around to it in a few days.)

Comment 1

[Brendan Eich [:brendan]]

Attachment: fix

Comment 2

[Brendan Eich [:brendan]]

Comment on attachment 371270 [details] [diff] [review]
fix

All the DELEGATE optimization pain was to avoid useless purges, which showed up on perf radar. I will try to get fresh radar data, but it seems worth taking this to reduce risk of sub-3.0 perf for code that hits the premature js_LeaveTrace call in current tip.

/be

Comment 3

[Brendan Eich [:brendan]]

Fixed on tm:

http://hg.mozilla.org/tracemonkey/rev/b1c0c4da1b88

I'm not forgetting about finding perf effects, but I'm gonna focus on other bugs for today's deadline. This bug stays open until m-c landing (I'm guessing that would happen no earlier than tomorrow); I should have some perf results by then.

/be

Comment 4

[Robert Sayre]

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

Comment 5

[Brendan Eich [:brendan]]

No perf results, d'oh. Maybe later, happy not to take risk by deferring this fix. Life's too short.

/be

Comment 6

[Robert Sayre]

We'll need perf results before we take this on 1.9.1

Comment 7

[Mike Beltzner [:beltzner, not reading bugmail]]

Comment on attachment 371270 [details] [diff] [review]
fix

As per sayrer's comment, can't approve until we know what this does, I guess? Please renom if you feel that assertion's bogus or if you have those results.

Comment 8

[Jason Orendorff [:jorendorff]]

Without this fix, we can bail off trace for no good reason whenever a script creates a new property.

graphs-new.m.o shows no visible change in Tracemonkey SunSpider numbers around April 6.

Comment 9

[Jason Orendorff [:jorendorff]]

(In reply to comment #8)
> Without this fix, we can bail off trace for no good reason whenever a script
> creates a new property.

Er, a new property on an object with the DELEGATE flag set, which helps explain why there was no measurable perf win.

Comment 10

[Brendan Eich [:brendan]]

(In reply to comment #9)
> (In reply to comment #8)
> > Without this fix, we can bail off trace for no good reason whenever a script
> > creates a new property.
> 
> Er, a new property on an object with the DELEGATE flag set, which helps explain
> why there was no measurable perf win.

With the DELEGATE flag bug-fixed (twice), we don't have any benchmarks that test for this fix's benefit. It would be interesting to debug gmail, though, and see if we ever hit the premature js_LeaveTrace without the patch. I'll do that now.

/be

Comment 11

[Brendan Eich [:brendan]]

(gdb) bt
#0  js_PurgeScopeChainHelper (cx=0x837a00, obj=0x500c8180, id=344207396) at ../../../tracemonkey/js/src/jsobj.cpp:3619
#1  0x00307749 in js_PurgeScopeChain (cx=0x837a00, obj=0x500c8180, id=344207396) at jsobj.h:608
#2  0x00308259 in js_DefineNativeProperty (cx=0x837a00, obj=0x500c8180, id=344207396, value=1342997536, getter=0, setter=0, attrs=7, flags=0, shortid=0, propp=0x0, entryp=0x0) at ../../../tracemonkey/js/src/jsobj.cpp:3770
#3  0x00309318 in js_DefineProperty (cx=0x837a00, obj=0x500c8180, id=344207396, value=1342997536, getter=0, setter=0, attrs=7, propp=0x0) at ../../../tracemonkey/js/src/jsobj.cpp:3680
#4  0x0026eac8 in DefinePropertyById (cx=0x837a00, obj=0x500c8180, id=344207396, value=1342997536, getter=0, setter=0, attrs=7, flags=0, tinyid=0) at ../../../tracemonkey/js/src/jsapi.cpp:3024
#5  0x0026f1ce in JS_DefinePropertyById (cx=0x837a00, obj=0x500c8180, id=344207396, value=1342997536, getter=0, setter=0, attrs=7) at ../../../tracemonkey/js/src/jsapi.cpp:3144
#6  0x145bbda2 in DefinePropertyIfFound (ccx=@0xbfffc2e0, obj=0x500c8180, idval=344207396, set=0x14ba2bf0, iface=0x14adb0e0, member=0x14adb104, scope=0x14a429a0, reflectToStringAndToSource=1, wrapperToReflectInterfaceNames=0x54b0bba0, wrapperToReflectDoubleWrap=0x54b0bba0, scriptableInfo=0x0, propFlags=7, resolved=0x0) at ../../../../../tracemonkey/js/src/xpconnect/src/xpcwrappednativejsops.cpp:464
#7  0x145bc1d2 in XPC_WN_NoHelper_Resolve (cx=0x837a00, obj=0x500c8180, idval=344207396) at ../../../../../tracemonkey/js/src/xpconnect/src/xpcwrappednativejsops.cpp:729
#8  0x0030580b in js_LookupPropertyWithFlags (cx=0x837a00, obj=0x500c8180, id=344207396, flags=65535, objp=0xbfffc470, propp=0xbfffc46c) at ../../../tracemonkey/js/src/jsobj.cpp:3963
#11 0x002d87cf in js_Interpret (cx=0x837a00) at ../../../tracemonkey/js/src/jsinterp.cpp:4575
etc.

(gdb) fr 11
#11 0x002d87cf in js_Interpret (cx=0x837a00) at ../../../tracemonkey/js/src/jsinterp.cpp:4575
4575	                if (!js_GetMethod(cx, obj, id, &rval, entry ? &entry : NULL))
(gdb) p script.filename
$1 = 0x14a44131 "file:///Users/brendaneich/Hacking/hg.mozilla.org/tracemonkey.OBJ/dist/MinefieldDebug.app/Contents/MacOS/components/nsMicrosummaryService.js"
(gdb) call js_FramePCToLineNumber (cx, fp)
$2 = 2208

Hitting this all the time, but I kept continuing with gmail loaded and hit

#0  js_PurgeScopeChainHelper (cx=0xe1c600, obj=0x1489e700, id=1339494796) at ../../../tracemonkey/js/src/jsobj.cpp:3619
#1  0x00307749 in js_PurgeScopeChain (cx=0xe1c600, obj=0x1489e700, id=1339494796) at jsobj.h:608
#2  0x00308259 in js_DefineNativeProperty (cx=0xe1c600, obj=0x1489e700, id=1339494796, value=1343700200, getter=0, setter=0, attrs=6, flags=0, shortid=0, propp=0x0, entryp=0x0) at ../../../tracemonkey/js/src/jsobj.cpp:3770
#3  0x002b63fa in js_GetCallObject (cx=0xe1c600, fp=0x57243b08) at ../../../tracemonkey/js/src/jsfun.cpp:640
#4  0x002dc609 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5097
#5  0x002f1cce in js_Invoke (cx=0xe1c600, argc=1, vp=0x57243ae4, flags=0) at jsinterp.cpp:1406
#6  0x002b7c8a in js_fun_call (cx=0xe1c600, argc=1, vp=0x57243ac4) at ../../../tracemonkey/js/src/jsfun.cpp:1685
#7  0x002dc9f6 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5171
#8  0x002f1cce in js_Invoke (cx=0xe1c600, argc=1, vp=0x57243308, flags=0) at jsinterp.cpp:1406
#9  0x002b7c8a in js_fun_call (cx=0xe1c600, argc=1, vp=0x572432e8) at ../../../tracemonkey/js/src/jsfun.cpp:1685
#10 0x002dc9f6 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5171
#11 0x002f1cce in js_Invoke (cx=0xe1c600, argc=1, vp=0x57243064, flags=0) at jsinterp.cpp:1406
#12 0x002b7c8a in js_fun_call (cx=0xe1c600, argc=1, vp=0x57243044) at ../../../tracemonkey/js/src/jsfun.cpp:1685
#13 0x002dc9f6 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5171
#14 0x002f1cce in js_Invoke (cx=0xe1c600, argc=0, vp=0x57242f24, flags=0) at jsinterp.cpp:1406
#15 0x002b8055 in js_fun_apply (cx=0xe1c600, argc=0, vp=0x57242f04) at ../../../tracemonkey/js/src/jsfun.cpp:1774
#16 0x002dc9f6 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5171
#17 0x002f1cce in js_Invoke (cx=0xe1c600, argc=0, vp=0x57242efc, flags=0) at jsinterp.cpp:1406
#18 0x002b8055 in js_fun_apply (cx=0xe1c600, argc=0, vp=0x57242edc) at ../../../tracemonkey/js/src/jsfun.cpp:1774
#19 0x002dc9f6 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5171
#20 0x002f1cce in js_Invoke (cx=0xe1c600, argc=1, vp=0x57242e4c, flags=0) at jsinterp.cpp:1406
#21 0x002b8055 in js_fun_apply (cx=0xe1c600, argc=1, vp=0x57242e2c) at ../../../tracemonkey/js/src/jsfun.cpp:1774
#22 0x002dc9f6 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5171
#23 0x002f1cce in js_Invoke (cx=0xe1c600, argc=1, vp=0x57242e20, flags=0) at jsinterp.cpp:1406
#24 0x145a7bd4 in nsXPCWrappedJSClass::CallMethod (this=0x4f8c96f0, wrapper=0x520d7c80, methodIndex=3, info=0x9516c8, nativeParams=0xbfffd274) at ../../../../../tracemonkey/js/src/xpconnect/src/xpcwrappedjsclass.cpp:1608


This is gmail (could you tell from all the .call and .apply nesting?):

(gdb) fr 4
#4  0x002dc609 in js_Interpret (cx=0xe1c600) at ../../../tracemonkey/js/src/jsinterp.cpp:5097
5097	                    if (JSFUN_HEAVYWEIGHT_TEST(fun->flags) &&
(gdb) p script.filename
$8 = 0x566e8f31 "http://mail.google.com/mail/?ui=2&view=js&name=js&ver=eTbnVk_09r0.en.&am=b9EwpczncKG5B91i0PQ2RmRyPaWRsA"
(gdb) call js_FramePCToLineNumber (cx, fp)
$9 = 501

My breakpoint was in js_PurgeScopeChainHelper exactly where js_LeaveTrace used to be, before this bug was fixed on tm (I'm running a tm tip build).

I then did this:

(gdb) i b
Num Type           Disp Enb Address    What
1   breakpoint     keep y   0x003076c3 in js_PurgeScopeChainHelper at ../../../tracemonkey/js/src/jsobj.cpp:3619
	breakpoint already hit 7 times
(gdb) cond 1 cx.thread.data.traceMonitor.onTrace
(gdb) c
Continuing.

and never hit the conditional breakpoint.

I still think this fix should be taken. But I don't have evidence that it is good for more than a teeny speedup to the already-hard case of purging (which may reshape if shadowing is going on).

/be

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 371270 [details] [diff] [review]
fix

We hit the premature js_LeaveTrace all the damn time (microformats, gmail, etc.). It just so happens that I don't see that happening while we are on trace.

For chrome JS we won't be on trace, of course. But why worry? The patch is safe and the upside of taking it is we eliminate the risk of tracing misfires.

Tracing needs to work reliably, not bail from a trace or abort a recording too often. We have struggled with the latter, with some bugs still showing too-high recording overhead even with better blacklisting.

I urge the powers that be to acknowledge the risk of misfires here, and the low risk of taking the patch (we have had this patch on tm and m-c for a week; code inspection also works in this case if you know the terrain).

/be

Comment 13

[Brendan Eich [:brendan]]

Comment 11, the gmail stack, shows a likely common DELEGATE case: Call objects. Even with upvar2. That js_DefineNativeProperty is binding the identifier for a named function expression in its declarative environment object, which has as its parent the global object or another Call object (or a Block, but gmail and other public web script does not use 'let' -- although catch blocks use implicit let).

This is why I worry about misfires.

/be

Comment 14

[Mike Shaver (:shaver emeritus)]

Comment on attachment 371270 [details] [diff] [review]
fix

a191=shaver -- Fennec will probably want chrome tracing, and we're not going to see perf effects with our current test load.

Comment 15

[Robert Sayre]

http://hg.mozilla.org/releases/mozilla-1.9.1/rev/b43d0deda2c1