445893 - Use the property cache for JSOP_NAMEINC, etc.

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Robert Sayre]

Good for a performance win.

See TM changeset 16321:ead9e74ecc9c.

Comment 1

[Brendan Eich [:brendan]]

http://hg.mozilla.org/tracemonkey/index.cgi/rev/ead9e74ecc9c

/be

Comment 2

[Brendan Eich [:brendan]]

Attachment: fix

Comment 3

[Mike Shaver (:shaver emeritus)]

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

Begone!

Comment 4

[Mike Shaver (:shaver emeritus)]

(By which I mean, "r=shaver".)

Comment 5

[Brendan Eich [:brendan]]

Fixed:

changeset:   16048:5c009a853d70
tag:         tip
user:        Brendan Eich <[e-mail]>
date:        Fri Jul 18 19:16:15 2008 -0700
summary:     Property-cache JSOP_NAMEINC etc. direct slot case (445893, r=shaver).

/be

Comment 6

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

Backed out:
http://hg.mozilla.org/mozilla-central/index.cgi/rev/337a43c87296

It hit a hang on the Windows tinderboxes (hang could mean crash dialog) and a fatal JS Assert (crash dialog) on my Windows debug build:

It was the (3) == len assert inside the DO_NEXT_OP(JSOP_INCNAME_LENGTH) line that you added -- that assert is only present in the !JS_THREADED_INTERP case, so it wouldn't show up on gcc-based platforms.

Comment 7

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The hang in question was running xpcshell unit tests -- in particular, I think while running xpcom/unit/test_bug374754.js.

Comment 8

[David Baron :dbaron: (⌚️UTC-4, no longer working on Mozilla)]

The stack of the assert was:

_DbgBreakPoint@0()	
JS_Assert(const char * s, const char * file, int ln) Line 59	C++
js_Interpret(JSContext * cx) Line 4013	C++
js_Execute(JSContext * cx, JSObject * chain, JSScript * script, JSStackFrame * down, unsigned int flags, long * result) Line 1550	C++
JS_ExecuteScript(JSContext * cx, JSObject * obj, JSScript * script, long * rval) Line 4932	C++
ProcessFile(JSContext * cx, JSObject * obj, const char * filename, _iobuf * file, int forceTTY) Line 673	C++
Process(JSContext * cx, JSObject * obj, const char * filename, int forceTTY) Line 752	C++
ProcessArgs(JSContext * cx, JSObject * obj, char * * argv, int argc) Line 871	C++
main(int argc, char * * argv, char * * envp) Line 1459	C++


In ProcessFile:

filename	0x00ee8f8d "c:/builds/mozilla-central/mozilla/tools/test-harness/xpcshell-simple/execute_test.js"	const char *


In JS_Assert:

+		s	0x101426fc "(3) == len"	const char *
+		file	0x101426c4 "c:/builds/mozilla-central/mozilla/js/src/jsinterp.cpp"	const char *



I still have it in the debugger if you need more.

Comment 9

[Brendan Eich [:brendan]]

Fixed again:

changeset:   16051:a7338e058704
tag:         tip
user:        Brendan Eich <[e-mail]>
date:        Fri Jul 18 22:53:22 2008 -0700
summary:     Property-cache JSOP_NAMEINC etc. direct slot case (445893, r=shaver).

The windows version of DO_NEXT_OP has a false degree of freedom -- its actual param must == len, if not be len. Grrr. Eagerly awaiting cross-platform inline + call threading!

/be