129496 - JS object initializer dynamic overhead (affects a DHTML test case)

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P2)

Description

[Markus Hübner]

See attached e-mail conversation ...

Comment 1

[Markus Hübner]

Attachment: E-Mail conversation

Comment 2

[Marc Boullet]

The URL is hanging the browser with 2002030603 or 2002030703 NT4.

Comment 3

[Brendan Eich [:brendan]]

It would help if we knew why this test URL allocates strings and objects like
mad.  I am pretty sure that even with generational GC, the testcase is going to
pay a high (but smoothed-out into little disruptions to the schedule) overhead
in GC.  If we find that it is just doing crasy O(n^2) or worse explosive growth
in JS object/string allocation, it should be fixed to do something smarter.  If
this has to do with DOM nodelists being document listeners, there's a bug on
that already, I believe.

Let's get to the bottom of this testcase, not just point fingers at the JS GC.

/be

Comment 4

[Randell Jesup [:jesup] (needinfo me)]

The base problem appears (at a glance) to be in this:

coord = function (x,y) { if(!x) var x=0; if(!y) var y=0; return {x: x, y: y}; } 

function getBezier(percent,C1,C2,C3,C4)
 { var pos = new coord(); 
   pos.x = ...
   pos.y = ...
   return pos;
}

Then lots of calls to getBezier().

Comment 5

[Brendan Eich [:brendan]]

>coord = function (x,y) { if(!x) var x=0; if(!y) var y=0; return {x: x, y: y}; } 
>
>function getBezier(percent,C1,C2,C3,C4)
> { var pos = new coord(); 
>   pos.x = ...
>   pos.y = ...
>   return pos;
>}

The new operator causes a new object to be made and passed as the this parameter
to the constructor, but in the case of coord, that object is wasted, because
coord returns an object created with an object initializer.  The JS engine could
do better here:
- it migth be able to analyze the constructor (this one is easy, there are
obvious hard cases) to see that it "overrides this" by returning a different object;
- it certainly could literalize a prototype object corresponding to the object
initializer (aka object literal) and avoid creating a new object with its own
scope for each coord call -- instead, it would clone the prototype and set its
slots.

Can someone try eliminating the operator new usage and see how much that helps?
 Also, any commentary on how 0.9.9-candidates (anything since bug 62164's patch
landed) does compared to 0.9.8?  It should help, but maybe not enough to matter
at the limit.

/be

Comment 6

[Markus Hübner]

Tried the testcase with 0.9.8 and 0.9.9 and both have the same problems as 
already described.

Comment 7

[Peter Nilsson]

Attachment: Updated testcase.

I'm not sure if this is what Brendan asked for in comment #5.
I elliminated the "new" operator in the getBezier function.

I couldn't measure any performance improvements..

Comment 8

[Brendan Eich [:brendan]]

Removing the new helps, but not enough -- there are simply too many new objects
created by the {x: x, y: y} literal.  I have a plan to fix that bloat issue in
bug 123668.

/be

Comment 9

[Markus Hübner]

reassigning ...

Comment 10

[Brendan Eich [:brendan]]

I should put a work-in-progress patch in here, just for backup purposes.  It
won't help on footprint yet.

/be

Comment 11

[Brendan Eich [:brendan]]

Moving out, some of these may move to 1.3alpha shortly.

/be

Comment 12

[Markus Hübner]

Bug 184352 seems to be affected by this too - dupe of this one?

Comment 13

[Brendan Eich [:brendan]]

bug 184352 is Mac-only, it can't have anything to do with this bug.

/be

Comment 14

[Brendan Eich [:brendan]]

Fixing TM.

/be

Comment 15

[Brendan Eich [:brendan]]

Noting true dependency.

/be

Comment 16

[Markus Hübner]

Can we get this into 1.4beta ?

Comment 17

[Brendan Eich [:brendan]]

Bump.

/be

Comment 18

[Brendan Eich [:brendan]]

Rather work on Narcissus.

/be

Comment 19

[Markus Hübner]

Testing the mentioned example at 
http://3dhtml.netzministerium.de/examples/beziercube3d.html
as well as all other examples at http://3dhtml.netzministerium.de/
like http://3dhtml.netzministerium.de/examples/solarsystem3d.html
with trunk build 2004112405 on winxp pro still is very choppy!
Maybe a profile shows were we are currently lacking.

Comment 20

[Brendan Eich [:brendan]]

This is low priority for a SpiderMonkey fix, but should be addressed in any new
runtime designed for JS2.

/be

Comment 21

[Markus Hübner]

Running with Mozilla/5.0 (Windows; U; Windows NT 5.1; en-US; rv:1.9a1) 
Gecko/20050907 Firefox/1.6a1 this is as it was. 
Also CPU usage is 70-80%

Comment 22

[Markus Hübner]

Updating testcase URL

Comment 23

[David Zbarsky (:dzbarsky)]

I have around 15% CPU usage but the animation is choppy.

Comment 24

[Brendan Eich [:brendan]]

The URL loads but nothing works, tons of ns and ie not defined errors. Anyone have a diagnosis or (better) fixed test?

/be

Comment 25

[Aiko]

Attachment: file:///d:/solarsystem3d.html

De-ns4-ified and all external files inlined version of SolarSystem3d.html.

Comment 26

[Brendan Eich [:brendan]]

Just wondering why the URL worked for crowder but not me -- anyone else have testimony?

/be

Comment 27

[Brendan Eich [:brendan]]

Attachment: micro-benchmarks testing initialisers vs. constructors

# baseline unpatched js shell:
$ ../src.bench/Darwin_OPT.OBJ/js /tmp/initests.js
1e6 small object initialisers: 1341
before 33316864, after 16384, break 07c05000
1e6 larger-than-fslots initialiser: 2375
before 33316864, after 16384, break 07c05000
1e6 small object constructions: 835
before 33316864, after 16384, break 07c05000
1e6 larger-than-fslots constructions: 1586

# with forthcoming patch:
$ ./Darwin_OPT.OBJ/js /tmp/initests.js
1e6 small object initialisers: 772
before 33316864, after 16384, break 07c05000
1e6 larger-than-fslots initialiser: 982
before 33316864, after 16384, break 07c05000
1e6 small object constructions: 882
before 33316864, after 16384, break 07c05000
1e6 larger-than-fslots constructions: 1307

Lopped 42% off the small fits-in-fslots case, 59% off the bigger case.

Note also the last number improving due to fslots-exceeded hazard elimination under JSOP_SETNAME/JSOP_SETPROP.

/be

Comment 28

[Brendan Eich [:brendan]]

Attachment: fix

Would appreciate anyone with time and interest applying this patch and testing it, solarsystem3d.html and general dogfooding. I will do that tomorrow but not today, trying to get a hard patch finished. Thanks,

/be

Comment 29

[Brendan Eich [:brendan]]

Should the URL link to the attachment now? I defer to bugzilla protocol experts.

/be

Comment 30

[Robert Sayre]

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

This crashed me.

Exception Type:  EXC_BAD_ACCESS (SIGBUS)
Exception Codes: KERN_PROTECTION_FAILURE at 0x0000000000000000
Crashed Thread:  0

Thread 0 Crashed:
0   libmozjs.dylib                	0x0013b2f2 ChangeScope + 139 (jsscope.c:361)
1   libmozjs.dylib                	0x0013c1b1 js_AddScopeProperty + 201 (jsscope.c:1024)
2   libmozjs.dylib                	0x00114340 js_SetPropertyHelper + 1087 (jsobj.c:3858)
3   libmozjs.dylib                	0x00101760 js_Interpret + 26860 (jsinterp.c:4374)
4   libmozjs.dylib                	0x0010770d js_Invoke + 2150 (jsinterp.c:1432)
5   libmozjs.dylib                	0x00107a8e js_InternalInvoke + 124 (jsinterp.c:1489)
6   libmozjs.dylib                	0x00107bc6 js_InternalGetOrSet + 200 (jsinterp.c:1546)
7   libmozjs.dylib                	0x0010ec29 js_NativeGet + 215 (jsobj.c:3523)
8   libmozjs.dylib                	0x00113e0a js_GetPropertyHelper + 674 (jsobj.c:3672)
9   libmozjs.dylib                	0x001008e0 js_Interpret + 23148 (jsinterp.c:4082)
10  libmozjs.dylib                	0x0010770d js_Invoke + 2150 (jsinterp.c:1432)
11  XUL                           	0x01a4c176 nsXPCWrappedJSClass::CallMethod

Comment 31

[Brendan Eich [:brendan]]

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

More a WIP, and backup. I'll debug.

/be

Comment 32

[Brendan Eich [:brendan]]

Attachment: fix, v2

This tests better, and I need to get it out of my tree and brain. Sayrer, can you give it a whirl? Thanks,

/be

Comment 33

[Brendan Eich [:brendan]]

Attachment: fix, v3

Removed another hazard in the {add,set,ini}pchit case, fixed last version's bogus hit test condition.

/be

Comment 34

[Robert Sayre]

This patch does not malfunction, but doesn't seem to help the tests in this bug.

Comment 35

[Brendan Eich [:brendan]]

Attachment: free slot on error

Comment 36

[Mike Shaver (:shaver emeritus)]

Comment on attachment 305305 [details] [diff] [review]
free slot on error

r=shaver, thanks for the call to go through this.

Comment 37

[Brendan Eich [:brendan]]

Attachment: fix bug found with further firefox testing

The clasp->reserveSlots hazard that the JSOP_SETNAME/JSOP_SETPROP logic faces was not addressed by previous patches. This patch resolves it by detecting when the cached sprop's slot does not match the next one allocated for obj, in which case it updates the cache entry. Tests well (mochi and suite plus random dogfooding), soliciting re-review.

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: interdiff of v3 vs. latest patch

Comment 39

[Brendan Eich [:brendan]]

Attachment: sigh, interdiff sucks, falling back on plain old diff

Comment 40

[Mike Shaver (:shaver emeritus)]

Comment on attachment 305879 [details] [diff] [review]
fix bug found with further firefox testing

Looks good, well-caught. r=shaver

Comment 41

[Brendan Eich [:brendan]]

Fixed on trunk:

js/src/jsapi.c 3.424
js/src/jsgc.c 3.283
js/src/jsgc.h 3.107
js/src/jsinterp.c 3.444
js/src/jsinterp.h 3.77
js/src/jsobj.c 3.443
js/src/jsopcode.tbl 3.109
js/src/jsscope.c 3.85
js/src/jsscope.h 3.57

Anything else to report about object initialisers (more could be done, probably under bug 412868), file a new bug after looking for existing to avoid dup'ing.

/be