123668 - Lots of mallocs from js_NewObject

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Simon Fraser [no longer active]]

The JS_malloc() in js_NewObject, which allocates the slots array for new JS 
Objects, does 6043 mallocs for startup + first browser window, allocating a total 
of 144996 bytes.

All these allocations are 24 bytes in size, apart from those for JavaArray, 
JavaClass, and JavaObject objects, which are 20 bytes (I was running on Mac with 
the MRJ Plugin installed).

This bug should address:
1) if we can reduce the number of JSObjects allocated, and
2) optimize allocation of obj->slots, perhaps by making it fixed size, or using
   arena allocation.

I'll attach a breakdown of the object classes.

Comment 1

[Simon Fraser [no longer active]]

Attachment: Counts of different JS object types for startup + browser window

Comment 2

[Phil Schwartau]

Reassigning to Kenton; cc'ing Roger -

Comment 3

[Brendan Eich [:brendan]]

>1) if we can reduce the number of JSObjects allocated, and

Simon: this bug may not be well-assigned, in the case where chrome JS analysis
and fixes are needed.  Should we have another bug, and maybe a tracking bug? 
For that matter, what tracking bug should depend on this one?

>2) optimize allocation of obj->slots, perhaps by making it fixed size, or
>using arena allocation.

Arenas are appropriate for LIFO allocation patterns, which do not describe JS
object slots allocations.  Arena + freelist (called a recycler by some around
here :-) might help, but only by cutting down on malloc overhead if all these
objects are indeed live after startup (and it seems most of them will be, see
below).

Shaver keeps hyping the slab allocator, mabye he'll take this bug.

I think the answer to question 1 is more important than this one.  It looks like
4679 out of 6043 objects are functions, which do need a parent slot (waldemar
asked today about why SpiderMonkey burns a word per object on the parent slot,
when only functions and DOM objects need it -- but most of these objects are
function and DOM objects), and 246 are XULElement objects (which I believe
require the silly DOM scope link).

An interesting question, one maybe sfraser can answer quickly, is how many of
these functions are actually called.  See bug 107907, the initial description --
the numbers there don't square with the ones attached here.

/be

Comment 4

[Brendan Eich [:brendan]]

I repeat that malloc is not evil, but JS could fuse an object's initial slots
with its GC-thing (arena-amortized) allocation.  I'll take this bug for 1.1.

/be

Comment 5

[Brendan Eich [:brendan]]

Doesn't seem like a beta change, but what the heck.

/be

Comment 6

[Brendan Eich [:brendan]]

I did some work on this over my vacation, but it's not going to make 1.1.

/be

Comment 7

[Markus Hübner]

brendan, what is the current status?

Comment 8

[Brendan Eich [:brendan]]

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

/be

Comment 9

[Brendan Eich [:brendan]]

Attachment: patch in progress, laying the groundwork

This should work, I tested lightly.  Phil, can you run it past the suites and
anything else?	Thanks, gotta crash.

/be

Comment 10

[Brendan Eich [:brendan]]

Attachment: don't finalize extension things unconditionally; no need to set flagp if thing is being coalesced to last_free_thing

Onward....

/be

Comment 11

[Brendan Eich [:brendan]]

Attachment: copy the right patch to the staging area, please

This is the patch that fixes a few bugs:

- Don't finalize extension-things unconditionally, only if we just finalized a
non-extension thing (this little bit of state machinery consolidates code, as
opposed to a scheme with an inner, inner loop that runs ahead of the finalized
thing's flagp).

- Don't set thing->flagp when freeing thing if we're coalescing it with the
last_free_thing (this seemed necessary only because of another bug I fixed just
before attaching the original patch, but it never made sense; it was late and I
wanted to get a working patch in the bug).

/be

Comment 12

[Phil Schwartau]

The latest patch passes the JS testsuite in both the debug/optimized
JS shell. No test regressions are introduced by the patch -

Comment 13

[Brendan Eich [:brendan]]

Attachment: latest and greatest

This ought to help quite a bit.  Testers, please measure if you can and report
improved memory use.

/be

Comment 14

[Brendan Eich [:brendan]]

Attachment: same as last patch, but with more cleanup and macro-ization

Anyone had time to try the last patch yet?

/be

Comment 15

[Phil Schwartau]

I will run the JS testsuite on it now -

Comment 16

[Phil Schwartau]

For some reason, my patch program is failing (silently) on the
latest attachment (in Comment #14). It stops after patching jsexn.c:

[//d/JS_EXP/mozilla/js/src] patch < 123668.patch
patching file `jsapi.c'
patching file `jsapi.h'
patching file `jsbool.c'
patching file `jscntxt.h'
patching file `jsdate.c'
patching file `jsexn.c'
[//d/JS_EXP/mozilla/js/src]


It should continue to patch jsfun.c, jsgc.c, etc., but it's not.
I will look into this problem -

Comment 17

[rogerl (gone)]

Patch worked fine for me - test suite ran fine (debug, win2k)

Comment 18

[Brendan Eich [:brendan]]

Attachment: patch updated to track jsstr.c change just checked in

Should work on a current trunk js/src.

Anyone have DHTML or pure JS performance test results?

/be

Comment 19

[Mazie Lobo]

System:  Windows 2000,  Memory 256 MB,  Processor:  1.7  GHz

I have applied the latest patch and tested in the optimized JS shell 
with loops like this, using object constructors:

function test()
{
  //Array
  var t0=new Date();
  for(x=0;x<1000000;x++)
  { 
    var arr=new Array();
  }
  print("Array:\t\t" +(new Date()-t0) + "\tms\n");

        etc.
        etc.
}

               BEFORE PATCH    AFTER PATCH
Array:          4453    ms      3204  ms
Object:         2781    ms      1718  ms
RegExp:         6953    ms      5172  ms
String:         2937    ms      1735  ms
Date:           3594    ms      2437  ms
Function:       14078   ms     10859  ms
Number:         3609    ms      2422  ms

Comment 20

[Mazie Lobo]

Same thing, only this time testing object literals:

function test()
{
  //Array
  var t0=new Date();
  for(x=0;x<1000000;x++)
  { 
    var a=[];
  }
  print ("Array:\t\t" +(new Date()-t0) + "\tms");

             etc.
             etc.
}

               BEFORE PATCH    AFTER PATCH
Array:          4953    ms       3250  ms
Object:         2984    ms       1750  ms
RegExp:         578     ms        563  ms
String:         579     ms        609  ms
Date:           n/a               n/a
Function:       2437    ms       1250  ms
Number:         578     ms        547  ms

Comment 21

[Brendan Eich [:brendan]]

Oops, is that x not declared as a local variable?

Can you post the whole testcase as an attachment to this bug?  Thanks.

/be

Comment 22

[timeless]

Attachment: xp.js

> xpcshell xp.js
Array:		64312	ms

> cscript /nologo xp.js
Array:		12338	ms

my xpcshell is unpatched from today. cscript is:
Microsoft (R) Windows Script Host Version 5.1 for Windows
[p2/450, 512mb of ram, xpcshell seemed to get 75-80% of the cpu]

Comment 23

[timeless]

Attachment: xp.js

> cscript /nologo xp.js
Array:		11056	ms
Object: 	11456	ms
RegExp: 	29052	ms
String: 	18266	ms
Date:		21451	ms
Function:		78844	ms
Number: 	11246	ms

(extra newlines stripped from xpcshell output)
> f:xpcshell xp.js
Array:		65014	ms
Object: 	50232	ms
RegExp: 	77742	ms
String: 	58273	ms
Date:		80586	ms
Function:		158949	ms
Number: 	65764	ms

Comment 24

[Mazie Lobo]

Attachment: Object constructor test, with Brendan's correction (var x)

Comment 25

[Mazie Lobo]

Attachment: Object literals test, with Brendan's correction (var x)

Comment 26

[Mazie Lobo]

These are the results I get when declaring the variable x inside the for-loop.  

Testing object constructors:

               BEFORE PATCH    AFTER PATCH
Array:          5375    ms      3250  ms
Object:         3078    ms      1657  ms
RegExp:         7813    ms      5297  ms
String:         3141    ms      1687  ms
Date:           3890    ms      2422  ms
Function:      16672    ms     11844  ms
Number:         3922    ms      2390  ms

Testing object literals:

               BEFORE PATCH    AFTER PATCH
Array:          5375    ms      3250  ms
Object:         3110    ms      1687  ms
RegExp:         437     ms       438  ms
String:         453     ms       453  ms
Date:            n/a             n/a 
Function:       2750    ms      1125  ms
Number:         422     ms       437  ms


I noticed in timeless' test, the object variable |arr| is global:
   t0=new Date(); for(x=0;x<1000000;x++) arr=new Array(); t1=(new Date()-t0);

In my test it is a local variable.

Comment 27

[Brendan Eich [:brendan]]

Comment on attachment 100294 [details]
Object constructor test, with Brendan's correction (var x)

The number test is calling new Date() in the loop.

/be

Comment 28

[Mazie Lobo]

Comment on attachment 100294 [details]
Object constructor test, with Brendan's correction (var x)

Sorry, I will add the corrected test case.

Comment 29

[Mazie Lobo]

Attachment: Object constructor test, with Brendan's correction (var x and number constructor)

Testing object constructors with updated number constructor:

	       BEFORE PATCH    AFTER PATCH
Array:		5672	ms	 1350  ms
Object: 	3406	ms	 1609  ms
RegExp: 	9047	ms	 5391  ms
String: 	3235	ms	 1687  ms
Date:		4359	ms	 2406  ms
Function:	18234	ms	11875  ms
Number: 	3422	ms	 1750  ms

Comment 30

[Mazie Lobo]

Today, I am getting an intermittent crash in the optimized JS shell 
with the latest patch applied.  It crashes in the |new Number()| loop,
but only when the JS shell is first started up.  That's probably why 
I didn't see this on Monday.  I had an error -|new Date()| instead of 
|new Number()|,  and I never restarted the JS shell after the correction.

Here is the crash:

The instruction at "0x44014444" referenced memory at "0x44014444".
The memory could not be "read".

Unhandled exception is js.exe:0xC0000005:Access Violation.

Call Stack in optimized JS shell with patch:
44014444()
JS32! js_SetClassPrototype + 87 bytes
JS32! JS_InitClass + 191 bytes
JS32! js_InitNumberClass + 83 bytes
JS32! JS_ResolveStandardClass + 346 bytes
JS! main + -45 bytes
JS32! js_LookupProperty + 628 bytes
JS32! js_FindProperty + 115 bytes
JS32! js_Interpret + 22997 bytes


I tried to crash in the Debug JS shell, but simply ran out of memory:

js>  
load("D:/Performance/169531-literals,constructors/169531-javascript-constructors-MILLION.js");
Array:          6594    ms
Object:         5953    ms
RegExp:         9344    ms
String:         93375   ms
Date:           124562  ms
D:/Performance/169531-literals,constructors/169531-javascript-constructors-MILLION.js:53:
out of memory
js> 
load("D:/Performance/169531-literals,constructors/169531-javascript-constructors-MILLION.js");
3: out of memory

Comment 31

[Brendan Eich [:brendan]]

Attachment: fixed the problem mazie found, and cleaned up a few unrelated nits

Please help me test this, I'd like to land it soon.  It passes mazie's test. 
Thanks, good find on that last crash!

/be

Comment 32

[Mazie Lobo]

Attachment: CScript object constructor test

Comment 33

[Mazie Lobo]

Attachment: CScript object literals test

Comment 34

[Mazie Lobo]

With Brendan's latest patch,  I no longer crash.

Here are my results with CScript, the optimized JS shell before and after the
latest patch, and the XPCshell from today's Mozilla trunk build (2002092508):

System:  Windows 2000,  Memory 256 MB,  Processor:  1.7  GHz 
CScript: Microsoft (R) Windows Script Host Version 5.6


Testing constructors:

              CScript       Opt JS(w/o patch)   Opt JS(w/patch)   Trunk XPCshell
Array:      2578   ms        3672  ms             2875 ms            11875 ms
Object:     2578   ms        2312  ms             1594 ms             9391 ms
RegExp:     8687   ms        5672  ms             4141 ms            14687 ms
String:     6797   ms        2187  ms             2344 ms            10735 ms
Date:       5563   ms        3141  ms             3031 ms            12578 ms
Function:  18859   ms       12891  ms            10172 ms            33406 ms
Number:     2672   ms        2656  ms             2640 ms            12500 ms


Testing literals:

              CScript       Opt JS(w/o patch)   Opt JS(w/patch)   Trunk XPCshell
Array:      2437   ms        3750  ms             2969 ms            12032 ms
Object:     2375   ms        2359  ms             1703 ms             9406 ms
RegExp:     2656   ms         406  ms              422 ms              312 ms
String:      532   ms         406  ms              406 ms              313 ms
Date:           n/a            n/a                 n/a                 n/a
Function:   3296   ms        1954  ms             1250 ms             9641 ms
Number:      422   ms         390  ms              390 ms              328 ms

Comment 35

[Phil Schwartau]

The latest patch passes the JS testsuite on WinNT in both the 
debug and optimized JS shell. No regressions were introduced.

However, I notice a new warning when building the shell:

  jsinterp.c(1434) : warning C4047: '=' : 'long ' differs in levels of 
  indirection from 'struct JSObject *'

From jsinterp.c:

          rval = OBJ_GET_PARENT(cx, withobj);       <------------- LINE 1434

Comment 36

[Brendan Eich [:brendan]]

Attachment: same as last but fixes jsinterp.c warning, adds ZZZ comment to jsapi.c

Comment 37

[Brendan Eich [:brendan]]

I'll have a new patch in a day or so, but this clearly missed 1.2.

/be

Comment 38

[Brendan Eich [:brendan]]

Attachment: latest patch

This patch uses a thread-private data structure containing a gc-thing freelist,
for a fast lock-free allocation path through js_AllocGCThing.  The
thread-private freelist is not built until about 8K of GC space has been
allocated by contexts active on the thread.  Once a thread has a private
freelist, it will be refreshed as the thread continues to allocate about 8K. 
This should remove most of the cost of thread safety in the allocator.

I could use lots of testing help, both to show correctness, and to benchmark js
vs. xpcshell vs. mozilla.  I need to run a debug xpcshell under MSVC on the
js/src/xpconnect/tests/js/old/threads.js testcase, which seems to hang for me
on Linux when run not under gdb, and which wedges earlier under gdb in a way
that leaves gdb unable to find the current thread id.  Maybe a thread died
without taking the whole app down?  Who knows.

Thanks for any help,

/be

Comment 39

[Phil Schwartau]

The latest patch passes the JS testsuite on WinNT in both the
debug and optimized shell. No test regressions were introduced.

Mazie is getting the performance data now -

Comment 40

[Markus Hübner]

No chance of getting this into 1.2 then?

Comment 41

[Brendan Eich [:brendan]]

Why put 1.2 at risk?  A patch this big is prime alpha material.  I expect more
change, followup fixes, further improvements, to be needed.  The latest patch is
not the one I'd check in today, or tomorrow.  Anyway, back to testing.

/be

Comment 42

[Mazie Lobo]

I have applied Brendan's latest patch which he posted in Comment #38.

Here are my results with CScript, optimized JS shell before and after the latest
patch, and 

XPCShell:

System:  Windows 2000,  Memory 256 MB,  Processor:  1.7  GHz 
CScript: Microsoft (R) Windows Script Host Version 5.6
Trunk XPCShell: Mozilla build ID-2002102108


Testing constructors:

             CScript       Opt JS(w/o patch)   Opt JS(w/patch)   Trunk XPCshell
Array:       2594 ms          3625 ms             2922 ms           11859 ms
Object:      2609 ms          2297 ms             1672 ms            9375 ms
RegExp:      8704 ms          5797 ms             4172 ms           14938 ms
String:      7125 ms          2281 ms             2265 ms           10703 ms
Date:        5219 ms          3109 ms             3125 ms           12391 ms
Function:   18859 ms         13016 ms            10016 ms           31844 ms
Number:      2594 ms          2593 ms             2734 ms           12609 ms


Testing literals:

              CScript       Opt JS(w/o patch)   Opt JS(w/patch)   Trunk XPCshell
Array:       2313 ms          3688 ms             3078 ms           11750 ms
Object:      2297 ms          2360 ms             1797 ms            9265 ms
RegExp:      2562 ms           390 ms              406 ms             313 ms
String:       532 ms           422 ms              407 ms             328 ms
Date:           n/a            n/a                 n/a                 n/a
Function:    3203 ms          1938 ms             1250 ms            9406 ms
Number:       390 ms           390 ms              390 ms             297 ms

Comment 43

[Dan Dickerman]

Attachment: jsobj.c with timing check as well as patch in attachment 103555

"cvs diff -u" of jsobj.c including current patch from this bug as well as
additional checks to benchmark timing in js_NewObject().  Put here as requested
in bug 171262 comment 5.  This measures time differences from start to return
of function and partitions-out the time taken in objectHook(), the last call
made before the return.  Counters are reset after 10sec of idleness, total
calls through here are counted to work-out per call averages.

Benchmarked with simple9999.html (attached to bug 171262) on a circa 1995 HP-UX
11.00 workstation, and taking the first and last reports...
stopwatch time 24 sec
js_NewObject	64 ..  0.006707s total	0.000105s ave per obj  56% for hook
js_NewObject  1408 .. 17.542389s total	0.012459s ave per obj  100% for hook

Interesting to note that the objectHook() portion is the substantial piece of
the timing for the objects in this test, and that the first allocations take
0.1ms, but 1400 objects later, allocations are taking 12.5ms (over 100x
longer).

Comment 44

[Dan Dickerman]

Test results for nums9999.html (attached to bug 171262) in the same config:
js_NewObject    64 ..   0.006632s total  0.000104s ave per obj   54% for hook
js_NewObject 21632 .. 435.227305s total  0.020120s ave per obj  100% for hook
Overall stopwatch time: 461sec
In this case, allcoation 21000 takes 20.1ms, 200x as long as the first few.

For reference, calling calloc(1,1024) 100,000 times in a loop on this machine
takes 41 seconds, averaging 0.4ms per call.

Comment 45

[Brendan Eich [:brendan]]

Dan, please disable venkman (type /startup-init off in its console and restart
mozilla -- you can verify that it's off at startup by starting venkman again and
typing /startup-init), then re-run the benchmark.  The runtime's objectHook
should be null, so your timing code should measure essentially zero time in the
hook.

Phil, would you please spin off a bug against the js debugger where this
apparent slowdown can be investigated separately from this bug?  Thanks.

/be

Comment 46

[Phil Schwartau]

I have filed bug 176087 against the JS Debugger on the above issue -

Comment 47

[Mazie Lobo]

I applied Brendan's latest patch and built the browser with this in my
mozconfig file:   ac_add_options --disable-debug
                  ac_add_options --enable-optimize=-O1

I have tried the tests with several different upper bounds. While running the
tests, I watched the Windows Task Manager and have made observations.

Here are the results with the patched and unpatched XPCshell:

                           Testing Constructors

Results with an upper bound of 10,000:

           Moz XPCshell(10-21-02)        XPCshell w/patch
Array:          78      ms                    78    ms
Object:         78      ms                    63    ms
RegExp:         110     ms                    93    ms
String:         62      ms                    63    ms
Date:           94      ms                    94    ms
Function:       234     ms                   218    ms
Number:         328     ms                    94    ms

Observations on Mozilla XPCshell: uses barely any RAM.
Observations on XPCshell w/patch: uses little RAM.


Results with an upper bound of 100,000:

           Moz XPCshell(10-21-02)        XPCshell w/patch
Array:          984     ms                   750    ms
Object:         906     ms                   650    ms
RegExp:         1532    ms                  1062    ms
String:         843     ms                   813    ms
Date:           1297    ms                  1250    ms
Function:       3360    ms                  3016    ms
Number:         1282    ms                  1234    ms

Observations on Mozilla XPCshell: uses barely any RAM.
Observations on XPCshell w/patch: uses half the RAM.


Results with an upper bound of 1,000,000 (bound of original tests):

           Moz XPCshell(10-21-02)        XPCshell w/patch
Array:          11782   ms                  10297    ms
Object:         9359    ms               couldn't continue: out of RAM
RegExp:         14516   ms
String:         10546   ms
Date:           12266   ms
Function:       32047   ms
Number:         12453   ms

Observations on Mozilla XPCshell: uses barely any RAM.
Observations on XPCshell w/patch: After 10 secs, almost all my RAM is used up, 
              
must be forced to quit.


The results for object literal creation were the same -

Comment 48

[Dan Dickerman]

Turns-out that disabling venkman isn't as easy as it should be (detailed in bug
176331), but once that was out of the way, I could gets some numbers on those
same tests with venkman disabled:
simple 9999
js_NewObject    64 .. 0.004076s total  0.000064s ave per obj  19% for hook
js_NewObject  1344 .. 0.059689s total  0.000044s ave per obj  28% for hook

nums9999
js_NewObject    64 .. 0.004232s total  0.000066s ave per obj  25% for hook
js_NewObject 21312 .. 1.209004s total  0.000057s ave per obj  23% for hook

Given that the average allocations started at 1/2 their previous starting-point
and stayed at roughly the same point, I have to believe that the venkman hook is
the major component to the problem here: following up with details for bug
176087.  Note also that the times here per-call are small enough that the calls
to gettimeofday() are showing up as a fairly significant part of the
measurement, as the "hook" would be null and not called in this case.

Comment 49

[Brendan Eich [:brendan]]

Regarding comment #47, something's clearly wrong with the patch.  The unpatched
xpcshell shouldn't use more memory that the patched one does.

Can you verify that the js shell, w/ and w/o the patch, works as reported in
comment #42 ?

Also, the unpatched xpcshell memory use should not be less than the js shell use
-- it ought to be a bit more, due to JS_THREADSAFE and XPCOM overheads.

/be

Comment 50

[Phil Schwartau]

I went over and retried the constructor tests in the optimized JS shell
on Mazie's machine, with and without the latest patch.

I observed the RAM as the tests ran, in each case starting from an
indicated value of 130M available RAM.

Results with an upper bound of 1,000,000 (bound of original tests):
Testing constructors:

             Opt JS(w/o patch)   Opt JS(w/patch)
Array:          3750    ms          2922    ms
Object:         2375    ms          1672    ms
RegExp:         5937    ms          4234    ms
String:         2313    ms          2266    ms
Date:           3250    ms          3141    ms
Function:       13234   ms         10484    ms
Number:         2656    ms          2781    ms

RAM:          130M ---> 114M      130M ---> 129M


The RAM behavior is exactly the opposite of what was observed in
the XPCshell. Here, in the optimized JS shell, less RAM is used
with the patch in place. By contrast, in the XPCshell, more RAM
was used with the patch in place; in fact, so much more that the
tests could not be run with the original upper bound of 1,000,000.

I don't know why that would be. In any case, the timings I got in
the JS shell are virtually the same as Mazie got in Comment #42 -

Comment 51

[Markus Hübner]

Does this still need tweaking or is it already ready to bake on the trunk?

Comment 52

[Brendan Eich [:brendan]]

Fixing TM.

/be

Comment 53

[Wenqing Jiang]

The bug, to be solved in 1.3 beta, depends on bug #176182, which will
be solved in 1.4 alpha.  Does that mean the bug can be solved before
176182, despite the dependency?

Comment 54

[Boris Zbarsky [:bzbarsky]]

no, it just means you should not put absolute trust in the target milestone field.

Comment 55

[Markus Hübner]

But this bug won't make 1.3b, right? :/

Comment 56

[Zibi Braniecki [:zbraniecki][:gandalf]]

no, because You should put absolute trust in the flags field :)

Comment 57

[Markus Hübner]

But this might make 1.3 final then?

Comment 58

[Brendan Eich [:brendan]]

If I can get my 1.4alpha-targeted patches landed in that milestone, I think I
can fit this into 1.4beta without adding too much risk.  Time will tell.

/be

Comment 59

[Brendan Eich [:brendan]]

Attachment: patch updated to trunk today

A diff -w version is next.

/be

Comment 60

[Brendan Eich [:brendan]]

Attachment: diff -w version of last patch

Comment 61

[Phil Schwartau]

The latest patch passes the JS testsuite for me on WinNT, in both
the debug and optimized JS shell. No test regressions are observed -

Comment 62

[Brendan Eich [:brendan]]

Attachment: fixed patch, please test heavily

This works for me on the constructors test.

/be

Comment 63

[Brendan Eich [:brendan]]

Attachment: diff -w of last patch

This one is for reviewing.

/be

Comment 64

[Phil Schwartau]

I have applied Brendan's latest patch in Comment #62.

Here are my results with CScript, the optimized JS shell before/after
the latest patch, and the XPCShell from a recent Mozilla trunk binary:

System:  WinNT 4.0, 128M RAM, 500 MHz CPU
CScript: Microsoft (R) Windows Script Host Version 5.6
Trunk XPCShell: Mozilla build ID 2003041609


Testing constructors:

             CScript       Opt JS(w/o patch)   Opt JS(w/patch)   Trunk XPCshell
Array:       5672 ms          10031 ms           9609 ms           32062 ms
Object:      5593 ms           4797 ms           4172 ms           24125 ms
RegExp:     19797 ms          12782 ms          13047 ms           38235 ms
String:     14750 ms           4765 ms           4797 ms           28062 ms 
Date:       11172 ms           6531 ms           5578 ms           32891 ms
Function:   52984 ms          36032 ms          35140 ms           89000 ms 
Number:      5766 ms           5125 ms           4610 ms           33234 ms  


Testing literals:

             CScript       Opt JS(w/o patch)   Opt JS(w/patch)   Trunk XPCshell
Array:       4641 ms          10110 ms           9766 ms           32078 ms
Object:      4531 ms           4875 ms           4250 ms           24125 ms
RegExp:      5391 ms            969 ms            953 ms            1141 ms
String:      1359 ms            969 ms            969 ms            1140 ms
Date:          n/a               n/a               n/a                n/a
Function:    6438 ms           3296 ms           2844 ms           21141 ms  
Number:      1046 ms            938 ms            922 ms            1078 ms

Comment 65

[Phil Schwartau]

The latest patch in Comment #62 also passes the JS testsuite on WinNT,
in both the debug and optimized JS shell. No test regressions occurred.

I will also build the browser with this patch in, and report back -

Comment 66

[Brendan Eich [:brendan]]

The patch doesn't pass
http://lxr.mozilla.org/mozilla/source/js/src/xpconnect/tests/js/old/threads.js,
alas.  I don't want to push this patch into a non-alpha release, so I'm going to
fix the remaining problems and land it in 1.5alpha, early.

/be

Comment 67

[Samir Gehani]

adt: nsbeta1-

Comment 68

[Markus Hübner]

1.5a freeze is 9th of july - can we get this in ?

Comment 69

[José Jeria]

Will this make it to 1.5b or will the new milestone be 1.6a?

Comment 70

[Brendan Eich [:brendan]]

Markush, there is no nsbeta1 keyword consumer any longer.  I'm removing it, lest
it cause confusion.

I hope to have a new patch here soon.

/be

Comment 71

[Brendan Eich [:brendan]]

Bump.

/be

Comment 72

[Markus Hübner]

When will this really be ready for baking on the trunk? (The TM is missed 
again)

Comment 73

[Nicholas Allen]

This is a trickier than it sounds bug.  Probably too tricky for anything but an
alpha release (and 1.8a4 is already packed with other feature work).  The
sticking point is that the allocator cannot afford much contention but still
needs to be memory efficient and handle multiple consumers.  There are some
design bugs, they need to be found, but teasing them out is going to take some
careful sleuthing about the concurrency.

Comment 74

[Zibi Braniecki [:zbraniecki][:gandalf]]

Can we say "it's for 1.9a1"?

Comment 75

[Markus Hübner]

This has been postponed from 1.2 on - so if this can make by any chance 1.8
(a4) that'll be highly appreciated.

Comment 76

[Zibi Braniecki [:zbraniecki][:gandalf]]

if the problem is lack of testing, we can try to involve as many testers as
possible, prepare compiled versions of branch with this patch so we can make
sure it's ready to serve.
I belive that this patch not only speeds up JS on websites, but also internal
Mozilla Suite parts that are written in JS.

Comment 77

[Markus Hübner]

Many people will contribute in finally getting this patch in. What can we do 
about it?!

Comment 78

[Brendan Eich [:brendan]]

Attachment: phase 1: size-segregated freelist, js_AllocGCThing extra param (to apply)

This is a lot simpler, and paves the way for a fix here and for other bugs.

/be

Comment 79

[Brendan Eich [:brendan]]

diff -w of that patch coming next.

/be

Comment 80

[Brendan Eich [:brendan]]

Attachment: phase 1 diff -w version (to review)

The patch this obsoletes would fragment like crazy, it was a bad idea.

/be

Comment 81

[Brendan Eich [:brendan]]

Oh, one more thing.  The patch at attachment 170117 [details] [diff] [review] works only if you turn off
E4X (#define JS_HAS_XML_SUPPORT 0 in jsconfig.h for the JS_VERSION == 150 case).

I'll have a patch for jsxml.[ch] to make E4X work with this patch shortly.

/be

Comment 82

[Brendan Eich [:brendan]]

Comment on attachment 170118 [details] [diff] [review]
phase 1 diff -w version (to review)

That was just to whet your appetite ;-).  Full patch that passes all testsuites
in a little while.

/be

Comment 83

[Brendan Eich [:brendan]]

Attachment: phase 1, v2 (apply)

Money all around.

The jsxml.c changes look big, but they're pretty much "pattern matching": 
Since all JSXML, JSXMLQName, and JSXMLNamespace are GC-things now, no need to
get JSObject wrappers for them when referencing them from more than one
JSXMLArray element.  JSXMLArrayFinish doesn't need a dtor argument.  No manual
destroys of object-less privates on error return paths.  Etc.

/be

Comment 84

[Brendan Eich [:brendan]]

Attachment: diff -w, v2 (review)

Comment 85

[Brendan Eich [:brendan]]

I checked in the phase 1 patch, plus some warning fixes and an E4X PutProperty
fix that dmose reported, to JSGC_REVAMP_20050103_BRANCH.  On to phase 2!

/be

Comment 86

[Brendan Eich [:brendan]]

Attachment: phase 2: avoid malloc'ing slots for unmutated prototype clones (apply)

Comment 87

[Brendan Eich [:brendan]]

Attachment: phase 2: avoid malloc'ing slots, etc., diff -w (review)

The goal here is to preserve friend API compatibility by not changing JSObject,
but always allocating a JSNativeObject that contains strongly-typed members for
proto, parent, clasp, and private data, moving those out of obj->slots and
thereby avoiding malloc'ing slots altogether for unmutated prototype clones.

This required more OBJ_GET/SET_* macroization, so it looks big.

It may be that this loses; it certainly breaks recent work to prevent very long
object chains from crashing the GC, if the attacker chains via __proto__, due
to the special-case JSNativeObject member storage of what used to be slots.

I'm not checking this into the branch yet, just recording it here for review
and comments (testing and benchmarking, ideally).  I'd like to try an
alternative patch that leaves JSObject alone, while allocating short-ish slots
vectors from the GC (as well as JSFunctions).

/be

Comment 88

[Brendan Eich [:brendan]]

Attachment: alternate patch2: GC-allocate small obj->slots, and JSFunction (apply)

This is better.  It passes the regular and e4x testsuites.  The only issues
are:

1.  Does it speed up real-world apps?  It obviously reduces malloc calls
substantially.	Numbers in a bit.

2.  Can we stand to break the memory pressure API compatibility, requiring
callers of JS_NewRuntime (aka JS_Init) to pass a larger maxbytes parameter than
they have, if they don't pass 0xffffffff?  Because we're allocating small-ish
slots vectors, and JSFunctions, from the GC-heap, we'll need to bump
XPConnect's 4MB argument, probably to 16MB or so.

/be

Comment 89

[Brendan Eich [:brendan]]

Attachment: alternate patch2: GC-allocate small obj->slots, etc., diff -w (review)

Comment 90

[Brendan Eich [:brendan]]

Comment on attachment 170215 [details] [diff] [review]
phase 2: avoid malloc'ing slots, etc., diff -w (review)

This is not the way to go, violating the obj->slots uniformity is bad, and for
most objects (which mutate) you still malloc a slots vector.

/be

Comment 91

[Brendan Eich [:brendan]]

Comment on attachment 170167 [details] [diff] [review]
diff -w, v2 (review)

shaver gave r=him over IRC, he's still on vacation but popped in just in time.

/be

Comment 92

[Brendan Eich [:brendan]]

Comment on attachment 170275 [details] [diff] [review]
alternate patch2: GC-allocate small obj->slots, etc., diff -w (review)

Getting ready to land these patches, woohoo!

/be

Comment 93

[Brendan Eich [:brendan]]

Attachment: consolidated branch landing patch, with liveconnect fix

For testing.  The liveconnect problem was because it uses JSObjectOps with a
NULL mark hook.  The previous patch here marked small obj->slots vectors in the
native implementation of that hook, js_Mark.  I moved that marking step into
jsgc.c for all objects (MARK_GC_THING).

There's no way, given the layering, for any user of JS_NewObject or JSClass (or
JSObjectOps, curse it) to allocate slots other than by using the common jsobj.c
AllocSlots/FreeSlots stuff.  So it's ok for the GC to know that small-enough
slots are GC-allocated, and need to be marked.	For now, at any rate!

/be

Comment 94

[Brendan Eich [:brendan]]

Comment on attachment 170315 [details] [diff] [review]
consolidated branch landing patch, with liveconnect fix

Passes pageload test on Mac OS X.

/be

Comment 95

[Adam D. Moss]

Did this turn out to be a win?  Comment 88 indicates that it's not totally clear.

Comment 96

[Zibi Braniecki [:zbraniecki][:gandalf]]

I will try to show results of DHTML/JS tests for 2005.01.01 build and todays.

Comment 97

[Brendan Eich [:brendan]]

I addressed issue 2 in comment 88 by keeping separate account of bytes allocated
for "private" data (small objects' slots and for JSFunction private data), so as
not to increase memory pressure incompatibly.

Another bug I'm hoping to fix during 1.8, bug 157334, should help shave cycles
off the GC-thing allocator by avoiding the runtime's GC lock.

But does any of this beat good ol' malloc?  Benchmarking on various OSes is the
only way to say.

This patch was a small codesize win:

  libmozjs.so
  	Total:	       -352 (+2444/-2796)
  	Code:	       -417 (+2312/-2740)
  	Data:	        +65 (+132/-56)

Thanks to Gandalf for his help benchmarking, and I'm interested in the results,
but I bet we'll have a hard time showing that this patch sped anything up more
than a few percent.  It would be interesting to measure RSS or other dynamic
footprint measures.

/be

Comment 98

[Brendan Eich [:brendan]]

gandalf, anyone: any numbers to compare before vs. after?

tinderbox stats didn't change much AFAICT.  Marking FIXED, feel free to report
results here.  Any problems seemingly in the code I touched should go in new
bugs, as usual.

/be

Comment 99

[Wenqing Jiang]

I found this URL for javascript performance test:

http://www.24fun.com/downloadcenter/benchjs/benchjs.html

I don't know to which extent it is relevant to the bug discussed here but
here's the result I got, on a win2k Celeron  1.2G machine:

       IE     ff 1.0  ff 050120
test#1 3.245 10.846   3.014
test#2 3.355  2.133   1.462
test#3 1.422  1.071   1.051
test#4 1.272  1.091   1.031
test#5 1.522  0.681   0.641
test#6 2.844  5.588   3.425
test#7 3.284  2.644   2.644

Comment 100

[Boris Zbarsky [:bzbarsky]]

> I don't know to which extent it is relevant to the bug discussed here

It's not. In fact, it's not relevant to JavaScript performance at all.  All
those are tests of DOM performance.

Also, note that if you search on that URL we have bugs for tracking performance
on those tests...

Comment 101

[Wenqing Jiang]

The fixed bug depends on bug 176182 but the latter is not fixed yet.  Is the fix
just a workaround?

Comment 102

[Brendan Eich [:brendan]]

Bogus dependency.

/be