420426 - 10% sunspider regression as a result of bug 418069

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Blake Kaplan (:mrbkap) (inactive)]

http://graphs.mozilla.org/graph.html#spst=range&spstart=1203017681&spend=1204353222&bpst=Cursor&bpstart=1203017681&bpend=1204353222&m1tid=151821&m1bl=0&m1avg=0

Comment 1

[Brendan Eich [:brendan]]

Attachment: attempt to get ComputeThis out of all critical paths

Comment 2

[Brendan Eich [:brendan]]

I left ComputeThis static in jsinterp.c and called only from js_ComputeThis. It's a tail call, gcc on Mac seems to inline ComputeThis on this account. I can inline by hand if you insist, but I'd rather not at this stage.

/be

Comment 3

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 306672 [details] [diff] [review]
attempt to get ComputeThis out of all critical paths

It'd be good to run this on the JS testsuite, but it looks good to me.

Comment 4

[Brendan Eich [:brendan]]

Fixed:

js/src/jsinterp.c 3.454

/be

Comment 5

[Andrew Schultz]

nye (MozillaTest) hit the assertion:

Assertion failure: !obj->map->ops->thisObject, at /home/andrew/tbox/SeaMonkey-mochi/Linux_2.6.22.14-72.fc6_Depend/mozilla/js/src/jsinterp.c:4208

running dom/tests/mochitest/ajax/prototype/test_Prototype.html

Comment 6

[Bob Clary [:bc] (inactive)]

failures in shell and windows:

<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1204357140.1204358350.24510.gz&fulltext=1>

<http://tinderbox.mozilla.org/showlog.cgi?log=MozillaTest/1204357140.1204358318.24418.gz&fulltext=1>

e4x/Regress/regress-370016.js EXIT STATUS: CRASHED 0 signal 6, reason: Assertion failure: OBJECT_IS_XML(cx, obj), at jsinterp.c:6324 BUGNUMBER: 370016 STATUS: with (nonxmlobj) function::

ecma/ExecutionContexts/10.1.4-10.js expected: Expected exit 0 actual: Actual exit 3, signal 0 reason: ./ecma/ExecutionContexts/10.1.4-10.js:91: TypeError: Number.prototype.toString called on incompatible With 10.1.4-10 Scope Chain and Identifier Resolution 

ecma/ExecutionContexts/10.2.2-2.js eval(new MyObject('hello').getClass()) expected: [object Object] actual: [object With] reason: wrong value

ecma/ExecutionContexts/10.2.2-2.js new MyObject('hello').getClass() expected: [object Object] actual: [object With] reason: wrong value

ecma/GlobalObject/15.1.2.1-2.js expected: Expected exit 0 actual: Actual exit 3, signal 0 reason: ./ecma/GlobalObject/15.1.2.1-2.js:64: TypeError: Date.prototype.getUTCMonth called on incompatible With 15.1.2.1-2 eval(x)

ecma/Statements/12.10.js expected: Expected exit 0 actual: Actual exit 3, signal 0 reason: ./ecma/Statements/12.10.js:59: TypeError: Number.prototype.valueOf called on incompatible With 12.10-1 The with statement

ecma_2/Statements/try-006.js EXIT STATUS: CRASHED 0 signal 6, reason: Assertion failure: !obj->map->ops->thisObject, at jsinterp.c:4208 try-006 The try statement

js1_5/Scope/scope-003.js EXIT STATUS: CRASHED 0 signal 6, reason: Assertion failure: OBJ_GET_CLASS(cx, obj) != &js_CallClass, at jsinterp.c:4207

this needs to come back out.

Comment 7

[Bob Clary [:bc] (inactive)]

backed out. shoot me when you wake up.

cvs update -j3.454 -j3.453 jsinterp.c
RCS file: /cvsroot/mozilla/js/src/jsinterp.c,v
retrieving revision 3.454
retrieving revision 3.453
Merging differences between 3.454 and 3.453 into jsinterp.c

Checking in jsinterp.c;
/cvsroot/mozilla/js/src/jsinterp.c,v  <--  jsinterp.c
new revision: 3.455; previous revision: 3.454

Comment 8

[Brendan Eich [:brendan]]

Attachment: consolidated patch, fixes all bugs

bc, would appreciate your testing this to pieces.

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: oops, right patch here

Comment 10

[Brendan Eich [:brendan]]

Attachment: diff -w version of last attachment

Comment 11

[Bob Clary [:bc] (inactive)]

On linux, I tested the regressions 

e4x/Regress/regress-354998.js
e4x/Regress/regress-370016.js
ecma_2/Statements/try-006.js
ecma/ExecutionContexts/10.1.4-10.js
ecma/ExecutionContexts/10.2.2-2.js
ecma/GlobalObject/15.1.2.1-2.js
ecma/ObjectObjects/15.2.4.2.js
ecma/Statements/12.10.js
js1_5/Scope/scope-003.js

with the original patch here (this.patch) and the patch in bug 420426 (more-this.patch) and the shell passed, the browser failed 

ecma/ObjectObjects/15.2.4.2.js

myvar = this;  myvar.toString = Object.prototype.toString; myvar.toString() expected: [object Window] actual: [object XPCCrossOriginWrapper] reason: wrong value

var MYVAR = new Object( this ); MYVAR.toString() expected: [object Window] actual: [object XPCCrossOriginWrapper] reason: wrong value

I think that is unrelated to this bug since I see them (and other regression) in the opt browser tests from after the backout.

I haven't done a full test run though. 

Comment 12

[Brendan Eich [:brendan]]

Comment on attachment 306755 [details] [diff] [review]
oops, right patch here

bc confirmed over irc that this fixes the js testsuite regressions. Need to unregress Tsspider stat!

/be

Comment 13

[Mike Connor [:mconnor]]

Comment on attachment 306755 [details] [diff] [review]
oops, right patch here

a=mconnor on behalf of drivers

Comment 14

[Brendan Eich [:brendan]]

Fixed:

js/src/jsapi.h 3.186
js/src/jsinterp.c 3.456
js/src/jsinterp.h 3.78
js/src/jsnum.c 3.109

/be

Comment 15

[Bob Clary [:bc] (inactive)]

tests covered by sunspider and comment 11

Comment 16

[Bob Clary [:bc] (inactive)]

v 1.9.0