Closed Bug 560358 Opened 14 years ago Closed 14 years ago

TM: cx->regExpStatics saving/restore a major bottleneck for string-unpack-code

Categories

(Core :: JavaScript Engine, defect, P1)

Product:
Core
Component:
JavaScript Engine
Type:
defect

Tracking

()

Status:
RESOLVED FIXED
Tracking Flags:
Tracking Status
blocking2.0 --- beta1+

People

(Reporter: gal, Assigned: gal)

References

Details

(Whiteboard: fixed-in-tracemonkey)

Attachments

(3 files, 1 obsolete file)

patch
18.03 KB, patch
brendan
: review+
Details | Diff | Splinter Review
patch
1.48 KB, patch
brendan
: review+
Details | Diff | Splinter Review
patch
3.82 KB, patch
brendan
: review+
Details | Diff | Splinter Review 
1869	      lambda_out:		
	1870	        if (freeMoreParens)		
	1871	            cx->free(cx->regExpStatics.moreParens);		
26.6%	1872	        cx->regExpStatics = save;		
	1873	        return ok;		
	1874	    }		

It seems to be a combination of memcpy overhead and L2 cache misses.
string-unpack-code saves and restores regExpStatics 81474 times (so 81474 * 2 memcpies).
Assignee: general → gal
Attached patch patch (obsolete) — — Splinter Review 

    
    

    
  
The attached patch makes JSRegExpStatics a pointer in JSContext. Its allocated when its written to, and deallocated when the context dies.

When calling into a regexp lambda function, the current state is pushed onto a stack and the state is set to NULL. This is a change from our previous behavior where we copied the state, and then restored it, overwriting any changes that happened. This is all completely unspecified behavior (before and after). If anyone relies on this, they are out of luck.

This optimization is a 2ms speedup on its own (unpack-code), and it takes care of a major source of L2 cache misses that another patch experienced (558754, butterfly effect).

    
    

    
  
Part of the reason that 558754 made such a big impact is that it subtly moved regExpStatics around relative to the start of JSRuntime and JSThreadData, making it no longer 32-byte aligned (which it was before accidentally). That made L2 cache misses more expensive since an extra cache line had to be fetched. We should patrol our code for memcpy and memset use and profile those areas a bit. Note that this memcpy was implicit (*a = *b), not explicit.

    
    

    
  
Alright so this doesn't really work as intended. We can't just clear the regexp statics and start with a fresh one.

This bug tracks back to here:

https://bugzilla.mozilla.org/show_bug.cgi?id=288688

To quote Brendan, "Quick and dirty patch next.  There's a better patch that should be done, but the patch I'll attach next fixes this, AFAICT."

I guess a good starting point is to back out the change, and then fix it differently using the test case as guidance what to fix.

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
This patch implements a saner memory handling for regExpStatics using luke's js::Vector, so we no longer have to save and restore the statics across lambda replace invocations. This mildly changes behavior for static fields of RegExp.prototype, but I think the previous behavior is just as insane as the new one, just differently insane.

        Attachment #440095 -
        Attachment is obsolete: true

    
  

        Attachment #440155 -
        Flags: review?(brendan)

    
  
Blocks: fastiterators

    
    

    
  
(In reply to comment #6)
> Created an attachment (id=440155) [details]
> patch
> 
> This patch implements a saner memory handling for regExpStatics using luke's
> js::Vector, so we no longer have to save and restore the statics across lambda
> replace invocations. This mildly changes behavior for static fields of
> RegExp.prototype,

RegExp, not RegExp.prototype.

> but I think the previous behavior is just as insane as the
> new one, just differently insane.

It doesn't matter what you think. Does this break the web?

http://www.google.com/codesearch?as_q=RegExp\.\%24&btnG=Search+Code&hl=en&as_lang=javascript&as_license_restrict=i&as_license=&as_package=&as_filename=&as_case=

I have no idea, but we should not just change semantics and make a wish. At least refine this code search and do some testing.

What do other browsers do?

/be

    
    

    
  
Comment on attachment 440155 [details] [diff] [review]
patch

>+            return false;
>+
>+        uintN i = 0;
>+        uintN n = cx->regExpStatics.parens.length();
>+        for (; i < n; i++) {

Could you scope uintN n to the for loop, make use of the init part of the for-head? Looks like n is not used after the loop.

Whew, great to have jsvector.h and C++ after all these years.

/be

        Attachment #440155 -
        Flags: review?(brendan) → review+

    
    

    
  
Test case:

javascript:function f() {"xxx".search(/(x)/);return "z";}"yyy".replace(/(y)/g, f);alert(RegExp.$1);

us without the patch: y
us with the patch: x
chrome: x
webkit/safari: x
opera: x
ie8: y

    
    

    
  
Thanks -- will r+ in a minute.

We should release-note the JS change based on comment 9 results, with us moving to the not-IE camp ;-).

/be
Keywords: relnote

    
    

    
  
Already plus'ed!

/be

    
    

    
  
http://hg.mozilla.org/tracemonkey/rev/35c25547a135
Whiteboard: fixed-in-tracemonkey

    
    

    
  
This needs beta coverage, so setting it to P1.
OS: Mac OS X → All
Priority: -- → P1
Hardware: x86 → All

    
  
blocking2.0: --- → beta1+

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/35c25547a135
Status: NEW → RESOLVED
Closed: 14 years ago
Resolution: --- → FIXED

    
    

    
  
Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.9.3a5pre) Gecko/20100425 Minefield/3.7a5pre

I believe this broke Facebook's Inbox.

Log in to Facebook. Click Messages. See no messages.

This changed between the build I'm using now and the nightly before. The changes include a merge with 59 TM changesets so I had to use TM hourlies from http://hourly-archive.localgho.st/hourly-archive2/tracemonkey-linux/ to find the regression window.

http://hg.mozilla.org/tracemonkey/pushloghtml?fromchange=63d72c1ee19f&tochange=35c25547a135

Error Console says Error: a is undefined
Source File: http://static.ak.fbcdn.net/rsrc.php/z5N5C/hash/dhdy6xq3.js
Line: 24

That long is ridiculously long, and probably doesn't tell you much as it looks like |a| comes from somewhere else.

DataStore=window.DataStore||{_storage:{},_elements:{},_tokenCounter:1,_NOT_IN_DOM_CONST:1,_getStorage:function(a){var b;if(typeof a=='string'){b='str_'+a;}else{b='elem_'+(a.__FB_TOKEN||(a.__FB_TOKEN=[DataStore._tokenCounter++]))[0];DataStore._elements[b]=a;}return DataStore._storage[b]||(DataStore._storage[b]={});},_shouldDeleteData:function(a){if(!a.nodeName)return false;try{if(null!=a.offsetParent)return false;}catch(ex){}if(document.documentElement.contains){return !document.documentElement.contains(a);}else return (document.documentElement.compareDocumentPosition(a)&DataStore._NOT_IN_DOM_CONST);},set:function(c,b,d){var a=DataStore._getStorage(c);a[b]=d;return c;},get:function(e,d,c){var b=DataStore._getStorage(e);var f=b[d];if((undefined===f)&&(typeof e.getAttribute=='function')){var a=e.getAttribute('data-'+d);f=(null===a)?undefined:a;}if((c!==undefined)&&(f===undefined))f=b[d]=c;return f;},remove:function(c,b){var a=DataStore._getStorage(c);delete a[b];return c;},cleanup:function(){var b,a;for(b in DataStore._elements){a=DataStore._elements[b];if(DataStore._shouldDeleteData(a)){delete DataStore._storage[b];delete DataStore._elements[b];}}}};

    
    

    
  
Thanks for the report. The source file you linked contains regexp manipulation, so I am inclined to believe this patch is implicated. Though, I can't see any manipulation of the static regexp state, which this patch touches. I will try to run this in a debug build and see what I can find.

    
  
Status: RESOLVED → REOPENED
Resolution: FIXED → ---

    
    

    
  
(In reply to comment #16)
> Thanks for the report. The source file you linked contains regexp manipulation,
> so I am inclined to believe this patch is implicated. Though, I can't see any
> manipulation of the static regexp state, which this patch touches. I will try
> to run this in a debug build and see what I can find.

I did builds using hg bisect, and it identified the following checkin as the regressor, which would seem to exonerate this bug as the cause:

The first bad revision is:
changeset:   41264:6bda799d061e
parent:      41263:72122e42712b
parent:      41205:498ea1457799
user:        Robert Sayre <sayrer@gmail.com>
date:        Sat Apr 24 12:56:26 2010 -0400
summary:     Merge mozilla-central to tracemonkey.

    
    

    
  
Nope, that's the changeset that merged this change onto m-c; you've added to the evidence blaming it. :-)

    
    

    
  
Yes, I should have realized that hg bisect kind of treated the entire thing as one change.  I will try backing this change out instead and report back.

    
    

    
  
I believe this change also broke the server status table on http://wonderproxy.com which is populated by appending table rows via JS. Current builds only display the flag of the proxy's location, ones without this change display the location and status icon.

I don't see any regexp usage in the JS though:
http://wonderproxy.com/js/addServerStatus.js

    
    

    
  
OK.  I have verified that backing out the patch for this bug fixes both the Facebook messages issue and the http://wonderproxy.com issue.
Depends on: 561700

    
    

    
  
I can reproduce the bug in facebook. I have confirmed that neither regexp_static_getProperty nor regexp_static_setProperty are hit while the bug happens, so whatever is going on here is not the change to RegExp we intended (that would be worse, it means we couldn't do the optimization we tried to do here). It must be an unintentional bug in the patch. Thanks for the confirmation Bill. This is very useful info.

    
    

    
  
I backed this out on mozilla-central --- see bug 561700. There were test failures that went unreported.
http://hg.mozilla.org/mozilla-central/rev/2968d19b0165

    
    

    
  
The backout is causing ctypes usage in Chrome Workers to fail. E.g.
http://tinderbox.mozilla.org/showlog.cgi?log=Firefox/1272268408.1272269169.24747.gz

We seem to be crashing in dtoa.c.

I'm at a loss to know what to do here. Those tests were enabled before this 
Andreas patch landed. I don't see anything that landed since this patch that looks like it could have interfered.

I could try backing out stuff randomly but that's probably not the best approach. At this point I think I'll just close the tree and wait for someone to come along and debug it. It's late here anyway.

    
    

    
  
There is a dependency problem in ctypes that manifests that way; can't find the bug number right now, but a clobber should resolve it.

    
  
Depends on: 561619

    
    

    
  
Bug 559056? I didn't know we knew it was a dependency problem.

    
    

    
  
The ctypes dependency bug was bug 561619. (Clearly shaver and I still read too much bugmail ;-).

/be

    
    

    
  
Well, that sucked. Fortunately I managed to go to bed instead of exploding in bitterness over losing my evening to this.

    
    

    
  
Dependency problem is pushed to TM; should merge to m-c pretty soon. Apologies that you spent time on this; I also sunk a day into it, without nailing down that it was a build problem.

Glad it's fixed now.

    
    

    
  
No worries, thanks for fixing it.

    
    

    
  
I decided to chase down where the a is undefined message comes from:

#0  js_ReportIsNullOrUndefined (cx=0x105470c00, spindex=1, v=22, fallback=0x0) at ../../../js/src/jscntxt.cpp:1754
#1  0x00000001040f8e68 in js_ValueToNonNullObject (cx=0x105470c00, v=22) at ../../../js/src/jsobj.cpp:6123
#2  0x00000001040c9ffc in js_Interpret (cx=0x105470c00) at jsops.cpp:1450
#3  0x00000001040e395d in js_Invoke (cx=0x105470c00, argc=1, vp=0x105c13ed8, flags=0) at jsinterp.cpp:842
#4  0x00000001040a68ee in js_fun_apply (cx=0x105470c00, argc=1, vp=0x105c13e60) at ../../../js/src/jsfun.cpp:2069
#5  0x00000001040ce4f7 in js_Interpret (cx=0x105470c00) at jsops.cpp:2210
#6  0x00000001040e395d in js_Invoke (cx=0x105470c00, argc=1, vp=0x105c13e38, flags=0) at jsinterp.cpp:842
#7  0x00000001040e3fec in js_InternalInvoke (cx=0x105470c00, obj=0x11e257280, fval=4796594240, flags=0, argc=1, argv=0x11c79ec40, rval=0x7fff5fbfcab8) at jsinterp.cpp:899
#8  0x0000000104046f61 in JS_CallFunctionValue (cx=0x105470c00, obj=0x11e257280, fval=4796594240, argc=1, argv=0x11c79ec40, rval=0x7fff5fbfcab8) at ../../../js/src/jsapi.cpp:4945
#9  0x0000000100990d39 in nsJSContext::CallEventHandler (this=0x1200c81b0, aTarget=0x11b42b400, aScope=0x11e257280, aHandler=0x11de63840, aargv=0x11c79ec08, arv=0x7fff5fbfcc70) at ../../../dom/base/nsJSEnvironment.cpp:2163
#10 0x00000001009c611f in nsGlobalWindow::RunTimeout (this=0x11b42b400, aTimeout=0x11c77eee0) at ../../../dom/base/nsGlobalWindow.cpp:8499
#11 0x00000001009c6672 in nsGlobalWindow::TimerCallback (aTimer=0x11c77ef40, aClosure=0x11c77eee0) at ../../../dom/base/nsGlobalWindow.cpp:8843
#12 0x000000010136aade in nsTimerImpl::Fire (this=0x11c77ef40) at ../../../xpcom/threads/nsTimerImpl.cpp:427
#13 0x000000010136acfc in nsTimerEvent::Run (this=0x11b6f0a70) at ../../../xpcom/threads/nsTimerImpl.cpp:519
#14 0x0000000101363f0e in nsThread::ProcessNextEvent (this=0x104b03980, mayWait=0, result=0x7fff5fbfcf14) at ../../../xpcom/threads/nsThread.cpp:527
#15 0x00000001012f2c1d in NS_ProcessPendingEvents_P (thread=0x104b03980, timeout=20) at nsThreadUtils.cpp:200
#16 0x00000001011a8010 in nsBaseAppShell::NativeEventCallback (this=0x116cc2df0) at ../../../../widget/src/xpwidgets/nsBaseAppShell.cpp:125
#17 0x000000010115ced4 in nsAppShell::ProcessGeckoEvents (aInfo=0x116cc2df0) at ../../../../widget/src/cocoa/nsAppShell.mm:394
#18 0x00007fff880bdf21 in __CFRunLoopDoSources0 ()
#19 0x00007fff880bc119 in __CFRunLoopRun ()
#20 0x00007fff880bb8df in CFRunLoopRunSpecific ()
#21 0x00007fff87a1aada in RunCurrentEventLoopInMode ()
#22 0x00007fff87a1a83d in ReceiveNextEventCommon ()
#23 0x00007fff87a1a798 in BlockUntilNextEventMatchingListInMode ()
#24 0x00007fff88229a2a in _DPSNextEvent ()
#25 0x00007fff88229379 in -[NSApplication nextEventMatchingMask:untilDate:inMode:dequeue:] ()
#26 0x00007fff881ef05b in -[NSApplication run] ()
#27 0x000000010115c773 in nsAppShell::Run (this=0x116cc2df0) at ../../../../widget/src/cocoa/nsAppShell.mm:747
#28 0x0000000100ed49f2 in nsAppStartup::Run (this=0x116ded0f0) at ../../../../../toolkit/components/startup/src/nsAppStartup.cpp:182
#29 0x0000000100027198 in XRE_main (argc=1, argv=0x7fff5fbfea70, aAppData=0x104b00340) at ../../../toolkit/xre/nsAppRunner.cpp:3536
#30 0x00000001000011f9 in main (argc=1, argv=0x7fff5fbfea70) at ../../../browser/app/nsBrowserApp.cpp:158

(gdb) call js_DumpStackFrame(cx->fp)
JSStackFrame at 0x105c146d0
<unnamed function at 0x11dde0620 (JSFunction at 0x11dde0620)>
file http://static.ak.fbcdn.net/rsrc.php/z5N5C/hash/dhdy6xq3.js line 24
  pc = 0x11b633649
  current op: getargprop
  slots: 0x105c14780
  sp:    0x105c14798 = slots + 3
    0x105c14780: undefined
    0x105c14788: "elem_"
    0x105c14790: undefined
  argv:  0x105c146a8 (argc: 1)
  this: <Object at 0x11e22e440>
  rval: undefined
  flags: none
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x105c145d0

JSStackFrame at 0x105c145d0
<unnamed function at 0x11dde0770 (JSFunction at 0x11dde0770)>
file http://static.ak.fbcdn.net/rsrc.php/z5N5C/hash/dhdy6xq3.js line 24
  pc = 0x11b633942
  current op: call
  slots: 0x105c14680
  sp:    0x105c146b0 = slots + 6
    0x105c14680: undefined
    0x105c14688: undefined
    0x105c14690: undefined
    0x105c14698: <unnamed function at 0x11dde0620 (JSFunction at 0x11dde0620)>
    0x105c146a0: <Object at 0x11e22e440>
    0x105c146a8: undefined
  argv:  0x105c14578 (argc: 3)
  this: <Object at 0x11e22e440>
  rval: undefined
  flags: none
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x105c14360

JSStackFrame at 0x105c14480
<unnamed function at 0x11e22e8c0 (JSFunction at 0x11dde0cb0)>
file http://static.ak.fbcdn.net/rsrc.php/z5N5C/hash/dhdy6xq3.js line 25
  pc = 0x11b634e42
  current op: call
  slots: 0x105c14530
  sp:    0x105c14590 = slots + 12
    0x105c14530: undefined
    0x105c14538: undefined
    0x105c14540: undefined
    0x105c14548: undefined
    0x105c14550: undefined
    0x105c14558: undefined
    0x105c14560: undefined
    0x105c14568: <unnamed function at 0x11dde0770 (JSFunction at 0x11dde0770)>
    0x105c14570: <Object at 0x11e22e440>
    0x105c14578: undefined
    0x105c14580: "Event.listeners"
    0x105c14588: <Object at 0x11de3cb80>
  argv:  0x105c14438 (argc: 3)
  this: <DOMPrototype object at 0x11e22e600>
  rval: undefined
  flags: none
  scopeChain: (JSObject *) 0x11e22e680
  displaySave: (JSStackFrame *) 0x7fff5fbfc750

JSStackFrame at 0x105c14360
<function ThreadRow at 0x11eb33bd0 (JSFunction at 0x11eb33bd0)>
file http://static.ak.fbcdn.net/rsrc.php/zBW19/hash/be7kdin2.js line 31
  pc = 0x11c851775
  current op: call
  slots: 0x105c14410
  sp:    0x105c14450 = slots + 8
    0x105c14410: <Array object at 0x11de3f9c0>
    0x105c14418: 3
    0x105c14420: undefined
    0x105c14428: <unnamed function at 0x11e22e8c0 (JSFunction at 0x11dde0cb0)>
    0x105c14430: <DOMPrototype object at 0x11e22e600>
    0x105c14438: undefined
    0x105c14440: "mouseover"
    0x105c14448: <unnamed function at 0x11de3cac0 (JSFunction at 0x11dda6f50)>
  argv:  0x105c14338 (argc: 2)
  this: <Object at 0x11de45f00>
  rval: undefined
  flags: constructing computed_this
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x105c14270

JSStackFrame at 0x105c14270
<unnamed function at 0x11df001c0 (JSFunction at 0x11eb37070)>
file http://static.ak.fbcdn.net/rsrc.php/zBW19/hash/be7kdin2.js line 35
  pc = 0x11c85e57c
  current op: new
  slots: 0x105c14320
  sp:    0x105c14348 = slots + 5
    0x105c14320: undefined
    0x105c14328: <function ThreadRow at 0x11eb33bd0 (JSFunction at 0x11eb33bd0)>
    0x105c14330: <Object at 0x11de45f00>
    0x105c14338: <Object at 0x11deedb00>
    0x105c14340: 1.47248e+12
  argv:  0x105c14248 (argc: 1)
  this: <Object at 0x11deedb00>
  rval: undefined
  flags: computed_this
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x105c14180

JSStackFrame at 0x105c14180
<unnamed function at 0x11df06d00 (JSFunction at 0x11eb32000)>
file http://static.ak.fbcdn.net/rsrc.php/zBW19/hash/be7kdin2.js line 26
  pc = 0x11c84c82e
  current op: call
  slots: 0x105c14230
  sp:    0x105c14250 = slots + 4
    0x105c14230: undefined
    0x105c14238: <unnamed function at 0x11df001c0 (JSFunction at 0x11eb37070)>
    0x105c14240: <Object at 0x11deedb00>
    0x105c14248: 1.47248e+12
  argv:  0x105c14148 (argc: 2)
  this: <Object at 0x11de58700>
  rval: undefined
  flags: computed_this
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x105c14060

JSStackFrame at 0x105c14060
<unnamed function at 0x11df06cc0 (JSFunction at 0x11eb31f50)>
file http://static.ak.fbcdn.net/rsrc.php/zBW19/hash/be7kdin2.js line 26
  pc = 0x11c84c6b8
  current op: call
  slots: 0x105c14110
  sp:    0x105c14158 = slots + 9
    0x105c14110: <Object at 0x11deea880>
    0x105c14118: 1.27232e+12
    0x105c14120: <Array object at 0x11de45ec0>
    0x105c14128: 0
    0x105c14130: 1.47248e+12
    0x105c14138: <unnamed function at 0x11df06d00 (JSFunction at 0x11eb32000)>
    0x105c14140: <Object at 0x11de58700>
    0x105c14148: 0
    0x105c14150: 1.47248e+12
  argv:  0x105c14028 (argc: 0)
  this: <Object at 0x11de58700>
  rval: undefined
  flags: computed_this
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x105c13f60

JSStackFrame at 0x105c13f60
<unnamed function at 0x11df06b40 (JSFunction at 0x11eb31cb0)>
file http://static.ak.fbcdn.net/rsrc.php/zBW19/hash/be7kdin2.js line 26
  pc = 0x11c84bea2
  current op: call
  slots: 0x105c14010
  sp:    0x105c14028 = slots + 3
    0x105c14010: 1.27232e+12
    0x105c14018: <unnamed function at 0x11df06cc0 (JSFunction at 0x11eb31f50)>
    0x105c14020: <Object at 0x11de58700>
  argv:  0x105c13f20 (argc: 0)
  this: <Object at 0x11de58700>
  rval: undefined
  flags: computed_this
  scopeChain: (JSObject *) 0x11e257280
  displaySave: (JSStackFrame *) 0x7fff5fbfbd20

JSStackFrame at 0x7fff5fbfbd20
<unnamed function at 0x11df00040 (JSFunction at 0x11eb36d90)>
file http://static.ak.fbcdn.net/rsrc.php/zBW19/hash/be7kdin2.js line 35
  pc = 0x10595fe70
  current op: call
  slots: 0x105c13ef0
  sp:    0x105c13f20 = slots + 6
    0x105c13ef0: null
    0x105c13ef8: undefined
    0x105c13f00: undefined
    0x105c13f08: undefined
    0x105c13f10: <unnamed function at 0x11df06b40 (JSFunction at 0x11eb31cb0)>
    0x105c13f18: <Object at 0x11de58700>
  argv:  0x105c13ee8 (argc: 1)
  this: <Object at 0x11deedb00>
  rval: undefined
  flags: computed_this rooted_argv
  scopeChain: (JSObject *) 0x11e257280

JSStackFrame at 0x7fff5fbfc750
<unnamed function at 0x11de63840 (JSFunction at 0x11dda6f50)>
file http://static.ak.fbcdn.net/rsrc.php/z49PH/hash/9p47jvzp.js line 11
  pc = 0x11b446afe
  current op: apply
  slots: 0x105c13e50
  sp:    0x105c13e80 = slots + 6
    0x105c13e50: <Object at 0x11deedb00>
    0x105c13e58: <Array object at 0x11de58540>
    0x105c13e60: <function apply at 0x11dd9d4d0 (JSFunction at 0x11dd9d4d0)>
    0x105c13e68: <unnamed function at 0x11df00040 (JSFunction at 0x11eb36d90)>
    0x105c13e70: <Object at 0x11deedb00>
    0x105c13e78: <Array object at 0x11de58540>
  argv:  0x105c13e48 (argc: 1)
  argsobj: <Object object at 0x11de58440>
  this: <Window object at 0x11e257280>
  rval: undefined
  flags: rooted_argv
  scopeChain: (JSObject *) 0x11de63680

function on top of the stack:

    _getStorage: function (a) {                                                                                                                                                 
        var b;                                                                                                                                                                  
        if (typeof a == 'string') {                                                                                                                                             
            b = 'str_' + a;                                                                                                                                                     
        } else {                                                                                                                                                                
            b = 'elem_' + (a.__FB_TOKEN || (a.__FB_TOKEN = [DataStore._tokenCounter++]))[0];                                                                                    
            DataStore._elements[b] = a;                                                                                                                                         
        }                                                                                                                                                                       
        return DataStore._storage[b] || (DataStore._storage[b] = {});                                                                                                           
    },                                                                                                                                                                          

so a is undefined here.

    
    

    
  
caller:

    get: function (e, d, c) {                                                                                                                                                   
        var b = DataStore._getStorage(e);                                                                                                                                       
        var f = b[d];                                                                                                                                                           
        if ((undefined === f) && (typeof e.getAttribute == 'function')) {                                                                                                       
            var a = e.getAttribute('data-' + d);                                                                                                                                
            f = (null === a) ? undefined : a;                                                                                                                                   
        }                                                                                                                                                                       
        if ((c !== undefined) && (f === undefined)) f = b[d] = c;                                                                                                               
        return f;                                                                                                                                                               
    },                                                                                                                                                                          

e is undefined ehre.

    
    

    
  
caller:

        listen: function (h, p, j, m) {                                                                                                                                         
            if (typeof h == 'string') h = $(h, true);                                                                                                                           
            if (typeof m == 'undefined') m = Event.Priority.NORMAL;                                                                                                             
            if (typeof p == 'object') {                                                                                                                                         
                var i = {};                                                                                                                                                     
                for (var o in p) i[o] = Event.listen(h, o, p[o], m);                                                                                                            
                return i;                                                                                                                                                       
            }                                                                                                                                                                   
            if (p.match(/^on/i)) throw new TypeError("Bad event name `" + event + "': use `click', not `onclick'.");                                                            
            p = p.toLowerCase();                                                                                                                                                
            var k = DataStore.get(h, b, {});
            if (f[p]) {                                                                                                                                                         
                var g = f[p];                                                                                                                                                   
                p = g.base;                                                                                                                                                     
                j = g.wrap(j);                                                                                                                                                  
            }                                                                                                                                                                   
            a(h, p);                                                                                                                                                            
            var q = k[p];                                                                                                                                                       
            if (!(m in q)) q[m] = [];                                                                                                                                           
            var l = q[m].length,                                                                                                                                                
                n = new EventHandlerRef(j, q[m], l);                                                                                                                            
            q[m].push(n);                                                                                                                                                       
            return n;                                                                                                                                                           
        },

    
    

    
  
  argv:  0x125ee9638 (argc: 3)

(gdb) x/3x 0x125ee9638
0x125ee9638:	0x00000016	0x00000000	0x1e3aad84
(gdb) p *(JSString*)0x1e3aad80
Cannot access memory at address 0x1e3aad80

So that's not a string alright.

    
    

    
  
Note that there was a call to match() immediately before this point. Maybe that clobbered some stuff?

    
    

    
  
Note, its all valid. I am just leotarded. 64-bit build, so I have to do x/3gx:

0x1054af638:	0x0000000000000016	0x000000011d69b3e4
0x1054af648:	0x000000011d69dd40

(gdb) x/3gx 0x1054af638
0x1054af638:	0x0000000000000016	0x000000011d69b3e4
0x1054af648:	0x000000011d69dd40
(gdb) call js_DumpValue(0x000000011d69b3e4)
jsval 0x11d69b3e4 = "mouseover"
(gdb) call js_DumpValue(0x000000011d69dd40)
jsval 0x11d69dd40 = <unnamed function at 0x11d69dd40 (JSFunction at 0x11dda94d0)>
(gdb)

    
    

    
  
function ThreadRow(a, e) {
    copy_properties(this, {
        threadId: e,
        _controller: a,
        _dirty: true,
        root: null,
        readLinks: [],
	authors: null,
        date: null,
        subject: null,
        preview: null,
        selector: null,
        selectorLabel: null
    });
    this._controller.threadCache.subscribe('GigaboxxThreadCache/dirty/' + e, this.dirty.bind(this), Arbiter.SUBSCRIBE_NEW);
    this.parent.construct(this, URI('/gigaboxx/templates/ThreadRow.tmpl'), {
        id: e
    });
    Event.listen(this.root, 'mouseover', CSS.addClass.curry(this.root, 'GBThreadRow_Hover'));

we are here ^

    Event.listen(this.root, 'mouseout', CSS.removeClass.curry(this.root, 'GBThreadRow_Hover'));
    var c = [this.selectorLabel, this.icon, this.badge, this.deleteButton];
    for (var d = 0; d < c.length; d++) {
        var b = c[d];
        Event.listen(b, 'mouseover', CSS.addClass.curry(this.root, 'GBThreadRow_HoverBlocker'));
        Event.listen(b, 'mouseout', CSS.removeClass.curry(this.root, 'GBThreadRow_HoverBlocker'));
    }
}

    
    

    
  
this is the following object, and it does not have a root property:

(gdb) call js_DumpObject(0x11ebf7e40)
object 0x11ebf7e40
class 0x1042ba440 Object
properties:
    enumerate "_errorView": slot 47
    enumerate "_messageHistoryView": slot 46
    enumerate "_threadListView": slot 45
    enumerate "_errorMsg": slot 44
    enumerate "_errorCode": slot 43
    enumerate "activeThreadMessagesRoot": slot 42
    enumerate "_activeReplyForm": slot 41
    enumerate "_hasShownMuffin": slot 40
    enumerate "_activeThreadData": slot 39
    enumerate "bottomMessagePager": slot 38
    enumerate "topMessagePager": slot 37
    enumerate "bottomPager": slot 36
    enumerate "topPager": slot 35
    enumerate "_messagePagerCollection": slot 34
    enumerate "_pagerCollection": slot 33
    enumerate "_threadList": slot 32
    enumerate "_resetPending": slot 31
    enumerate "_threadRows": slot 30
    enumerate "threadCache": slot 29
    enumerate "_viewStateData": slot 28
    enumerate "_savedSearches": slot 27
    enumerate "_syncCounts": slot 26
    enumerate "_pageSize": slot 25
    enumerate "_view": slot 24
    enumerate "_viewState": slot 23
    enumerate "_activeThread": slot 22
    enumerate "_counterSubscription": slot 21
    enumerate "_postPaintHooks": slot 20
    enumerate "_prePaintHooks": slot 19
    enumerate "_asyncPendingRequestQueue": slot 18
    enumerate "_asyncRequestQueue": slot 17
    enumerate "_blockLoadData": slot 16
    enumerate "_isDirty": slot 15
    enumerate "_state": slot 14
    enumerate "_undo_manager": slot 13
    enumerate "_contentArea": slot 12
    enumerate "toolbarManager": slot 11
    enumerate "_pageHeader": slot 10
    enumerate "_folderNav": slot 9
    enumerate "_config": slot 8
    enumerate "_originalPageTitle": slot 7
    enumerate "isDarkUser": slot 6
    enumerate "userId": slot 5
    enumerate "_arbiter": slot 4
    enumerate "STATE_MAP": slot 3
    enumerate "Debug": slot 2
proto <Object at 0x11ebcdac0>
parent <Window object at 0x11e0bb340>
slots:
   2 = <Object at 0x11ebf7e80>
   3 = <Object at 0x11ebf7ec0>
   4 = <Object at 0x11d600a80>
   5 = "6029215"
   6 = 0
   7 = "Facebook"
   8 = <Object at 0x11ebf7d40>
   9 = null
  10 = <HTMLDivElement object at 0x11ebf7d80>
  11 = <Object at 0x11d6006c0>
  12 = <HTMLDivElement object at 0x11ebf7c80>
  13 = <Object at 0x11d600fc0>
  14 = "PAINT"
  15 = false
  16 = false
  17 = <Array object at 0x11d601100>
  18 = <Array object at 0x11d601140>
  19 = <Array object at 0x11d601180>
  20 = <Array object at 0x11d6011c0>
  21 = <Object at 0x11d619bc0>
  22 = null
  23 = <Object at 0x11d601200>
  24 = "thread_list"
  25 = 20
  26 = false
  27 = <Object at 0x11d6015c0>
  28 = <Object at 0x11d601600>
  29 = <Object at 0x11d601640>
  30 = <Object at 0x11d601700>
  31 = false
  32 = <Object at 0x11d601a80>
  33 = <Object at 0x11d612dc0>
  34 = <Object at 0x11d612e80>
  35 = <Object at 0x11d604bc0>
  36 = <Object at 0x11d608740>
  37 = <Object at 0x11d60a940>
  38 = <Object at 0x11d60cb80>
  39 = null
  40 = false
  41 = null
  42 = null
  43 = null
  44 = null
  45 = <Object at 0x11d67e380>
  46 = <Object at 0x11d6912c0>
  47 = <Object at 0x11d691340>

    
    

    
  
my two favorite property names in this debugging session so far

    enumerate "_hasShownMuffin": slot 40
    enumerate "isDarkUser": slot 6

    
    

    
  
Looks like this gets fixed if I properly preserve the state of the regexp statics. Disgusting.
Keywords: dev-doc-needed, 
          
            relnote

    
  
Whiteboard: fixed-in-tracemonkey

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
preserve regexp statics

    
    

    
  
Comment on attachment 441651 [details] [diff] [review]
patch

>diff --git a/js/src/jsstr.cpp b/js/src/jsstr.cpp
>--- a/js/src/jsstr.cpp
>+++ b/js/src/jsstr.cpp
>@@ -1764,16 +1764,29 @@ PushRegExpSubstr(JSContext *cx, const JS
>     size_t off = sub.chars - whole->chars();
>     JSString *str = js_NewDependentString(cx, whole, off, sub.length);
>     if (!str)
>         return false;
>     *sp++ = STRING_TO_JSVAL(str);
>     return true;
> }
> 
>+class PreserveRegExpStatics {
>+    JSContext *cx;
>+    JSRegExpStatics save;

Blank line here.

>+  public:
>+    PreserveRegExpStatics(JSContext *cx) : cx(cx), save(cx) {
>+        save.copy(cx->regExpStatics);
>+    }
[snip]
>@@ -1795,16 +1808,18 @@ FindReplaceLength(JSContext *cx, Replace
> 
>         if (!rdata.invokevp) {
>             rdata.invokevp = js_AllocStack(cx, 2 + argc, &rdata.invokevpMark);
>             if (!rdata.invokevp)
>                 return false;
>         }
>         jsval* invokevp = rdata.invokevp;
> 
>+        PreserveRegExpStatics save(cx);
>+

Toldyaso... but wait, didn't *you* tell *me* this would break our Mozilla/IE backward compat by emulating WebKit?

/be

        Attachment #441651 -
        Flags: review+

    
    

    
  
I don't intend to land this patch with the fix. I will rewrite the regexp code to use a write barrier in the static code instead. test() just remembers the regexp+lastIndex and the string, not the match results. This is the only state that needs to be preserved. When the static state is actually used, I will reify it using the saved information (by re-executing the regexp). This will speed up the sane case (and probably SS) and penalize using the static state.

    
    

    
  
backed out of TM as well

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

    
    

    
  
Reexecuting the regexp has observable effects if //g, namely lastIndex. In any event re-executing may take a long time.

What is the performance effect of the patch you just attached?

/be

    
    

    
  
Yeah, of course, lastIndex has to be cached as well. Its a quad (input, reobj, lastIndex, match). match is lazily NULL initially. Re-executing in case someone uses RegExp.* seems ok as a de-optimized path. We should evangelize against the use of that. Its totally non-standed. I mocked up a patch. The biggest problem is leftContext and rightContext. Those can't be extracted from the match object, so I would have to save them somewhere else. That's uberlame.

I didn't measure but I think the additional patch would still keep things performance neutral.

The new proposal should be a win. Not clear by how much though. Maybe not worth it?

    
    

    
  
Smells like your approach will save about as many bytes, and cost in code size and complexity. Doesn't seem worth it, as a gut-check.

/be

    
    

    
  
Brendan is right. Don't optimize without profiles. I will land what we have, and we can do more later.

    
  
Whiteboard: fixed-in-tracemonkey

    
    

    
  
Waldo backed out.

Re-landing same patch. I caused a recompile that exposed a missing virtual destructor in some gc patch (I think).

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

    
    

    
  

      
      
      
      

        Attached patch
          
          
        patch
        — 
          — Splinter Review
      

    


  
The saga continues. Brendan, could I get a retroactive r+ for this? we have to poke the gc when losing the value of input.

    
  

        Attachment #442282 -
        Flags: review?(brendan)

    
    

    
  
Put the opening { on its own line!

Why did you remove the gcPoke?

/be

    
    

    
  
which one?

    
    

    
  
Comment on attachment 442282 [details] [diff] [review]
patch

I was asking why we lost a net poke, requiring the followup fix -- gal 'splained on irc.

The methods moved to out-of-line, even though clearRoots is small. I missed the reason for that. No biggie, just curious.

r=me with out-of-line method definition brace style fixed.

/be

        Attachment #442282 -
        Flags: review?(brendan) → review+

    
    

    
  
#56: JSContext wasn't defined at that place (it comes later), so cx->runtime didn't work. I could have moved them below JSContext. Maybe worth it. I will profile this a bit and see.

    
    

    
  
http://hg.mozilla.org/mozilla-central/rev/ac8dcb4886a9
Status: REOPENED → RESOLVED
Closed: 14 years ago → 14 years ago
Resolution: --- → FIXED

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






  

    
  







  

    

      
Attachment

      

      
      
      
      
    

    

      

        

          

            
General

            

              Creator: 



            

            
Created: 

            
Updated: 

            
Size: