109218 - js_SetupLocks appears excessive.

Status: VERIFIED FIXED

(Core :: JavaScript Engine, defect, P1)

Description

[Garrett Arch Blythe]

On just starting up the app, about:blank, and then shutting down js_SetupLocks
uses 85,120 bytes of memory, which lasts throughout the entire lifetime of the app.

The mojority of the memory is in 608 PR_NewLock calls, producing each an object
of 88 bytes in size (53,504 bytes).  This is the part I was wondering about....
 608 seems like a rather large number of synchronization objects.  Perhaps it is
needed, in which case close this bug.

The data is easy to reproduce, just use SpaceTrace and restrict the callsites to
contain the text "js_SetupLocks" in the options.  Then take a look at the top
callsites.

Comment 1

[Phil Schwartau]

Reassigning to Kenton -

Comment 2

[Cathleen]

Kenton, please address this bug, and assign to an appropriate milestone. -thanks.

(adding footprint keyword)

Comment 3

[Brendan Eich [:brendan]]

Garrett, can you please attach some stacks?  The way trace-malloc works on
Linux, you don't get static function names, so I believe js_SetupLocks is not to
blame here, rather ListOfFatlocks/NewFatlock are.  This is old bjorn code that
should not be called nearly so many times (608, as you note, seems high for the
PRLock count -- you should also see a PRCondVar for each such PRLock in the
spacetrace output -- do you?).

Kenton has inherited the JS engine code and is probably busy elsewhere on it. 
I'm happy to take this bug and do some more analysis, at least.  On the off
chance that no one will mind, I'm doing that now!

/be

Comment 4

[Garrett Arch Blythe]

Attachment: Shortened SpaceTrace output.

Here's a small snapshot of the SpaceTrace output.
Load js_SetupLocks/index.html and look at the Top Callsites Report to see what
I was looking at.

Comment 5

[Garrett Arch Blythe]

NOTE:  previous attatchement is a     .tar.gz

Comment 6

[Brendan Eich [:brendan]]

Attachment: easy fix, take advantage of lazy ListOfFatlocks allocation, don't eagerly pre-allocate fat-lock-lists

Comment 7

[Brendan Eich [:brendan]]

Looking for testing and review buddies.

/be

Comment 8

[John Bandhauer]

bjorn deserves a little slapping around for naming *anything* 'l'.

How about we avoid the warning for the first dip into the locks...
 #ifdef DEBUG
-        printf("Ran out of fat locks!\n");
+        if(fl_list_table[i].taken)
+            printf("Ran out of fat locks!\n");
 #endif
We're still likely to end allocating most of these lock, no? Maybe we should
crank down the numbers in the js_SetupLocks call for the runtime?

I'll run with this a bit and report back.

Comment 9

[Garrett Arch Blythe]

tested patch, evaluated under SpaceTrace:

js_SetupLocks takes a total of 3200 bytes on startup, about:blank, shutdown (vs
85120).  :)

Comment 10

[John Bandhauer]

Comment on attachment 58582 [details] [diff] [review]
easy fix, take advantage of lazy ListOfFatlocks allocation, don't eagerly pre-allocate fat-lock-lists 

sr=jband
I ran the xpcshell threads tests a bunch. They're happy. I note that even with
1000 iteration of 10 threads accessing some common data we rarely make any of
these fat locks (~4 sets of locks constructed).

Comment 11

[Brendan Eich [:brendan]]

Attachment: patch with jband's suggestions (both of 'em)

Comment 12

[John Bandhauer]

Comment on attachment 58632 [details] [diff] [review]
patch with jband's suggestions (both of 'em)

r/sr=jband

Comment 13

[Mike Shaver (:shaver -- probably not reading bugmail closely)]

Comment on attachment 58632 [details] [diff] [review]
patch with jband's suggestions (both of 'em)

Can you rename the l variable?	It took me, uh, longer than I want to admit
to realize that it wasn't a one.

With that, r=shaver.

Comment 14

[Brendan Eich [:brendan]]

Fix is in.

/be

Comment 15

[Phil Schwartau]

Based on Garrett's and jband's results in Comment #9, Comment #10,
respectively, marking Verified Fixed.