365048 - JSRuntime.threadTPIndex should be a static

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Peter Naulls]

User-Agent:       Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061024 Iceweasel/2.0 (Debian-2.0+dfsg-1)
Build Identifier: Mozilla/5.0 (X11; U; Linux i686; en-US; rv:1.8.1) Gecko/20061024 Iceweasel/2.0 (Debian-2.0+dfsg-1)

Because of the above limit, it is not possible to successfully call PR_SetThreadPrivate more than 128 times, even if you destroy the thread first.  This has follow on effects such as not being able to create more than 128 JavaScript runtimes sequentially.

Can this please be done properly - instead of a fixed array, at the very least a vector, or perhaps a hash table, or something.  


Reproducible: Always

Comment 1

[Wan-Teh Chang]

It should be PR_NewThreadPrivateIndex rather than PR_SetThreadPrivate
that you cannot successfully call more than 128 times.  Is this the
call that fails when you create more than 128 JavaScript runtimes
sequentially?

http://lxr.mozilla.org/seamonkey/source/js/src/jsapi.c#700

Thread private data indices are scarce resources.  Each library
should get only one thread private data index from NSPR.  If the
library has multiple thread private data items, it should aggregate
them in a structure and point the thread private data pointer at the
structure.  This additional level of indirection should solve the
JavaScript problem.

Comment 2

[Peter Naulls]

That is the code.  Can we therefore please assign it to the JS team, rather than having it marked as WONTFIX and vanishing into obscurity?  

Thanks.

Comment 3

[Wan-Teh Chang]

I changed this to a JavaScript bug.  The summary of the bug
needs to be changed after we know how JavaScript may create
too many NSPR thread private data indices/

Comment 4

[Brendan Eich [:brendan]]

Peter: what are the call stacks for the first few calls?  There should be at most one call per JSRuntime.

Are you using a JSRuntime per thread?  That's not desirable.  If it's unavoidable (others have done it in the past), we need to make JSRuntime.threadTPIndex static.

/be

Comment 5

[Peter Naulls]

I do have one runtime per thread, but there are generally only two threads.  What we're doing is processing many documents for content and using the JS engine to augment that.  A new JSRuntime is called for every document with JS in it, which is why we quickly run out. Are you suggesting reuse of the same runtime; that doesn't seem ideal.

I don't have a backtrace immediately to hand, sorry, but it is simply the sequence of calls from the code mentioned above. 

Comment 6

[Benjamin Smedberg]

Why do you need a separate JSRuntime for each document? Why can't you just keep the contexts separate?

Comment 7

[Brendan Eich [:brendan]]

Peter: you are leaking runtimes if you have only two threads but use up all 128 thread-private indexes.  Fix that leak, then you should use at most two TPIs from the JS engine.

You're also abusing runtimes. The runtime contains the GC heap, in which objects and other GC-things can be shared among threads.  It's typically one per process.  For thread safety, you need at least one JSContext per thread concurrently running scripts.  Typical architecture uses 1 JSRuntime per process and a pool of threads with contexts, to recycle rather than destroy and recreate up to some pool size limit.

/be

Comment 8

[Peter Naulls]

We're not leaking runtimes, there's no question of that.  The code in nspr is simply an atomic increment, regardless of JSRuntime clean up. 

This issue relates to #366446.   Ultimately, we need to limit the overall memory usage of the JSEngine on a per-document basis - not on a per-process basis.   

At present, we create a new JSRuntime per document.  I've talked to the guy who is in charge of our code that does we, and we've agreed that the best solution for us is to have one Runtime per thread.  This ties in with the patch given for #366466, and means the JSRuntime -> JSAllocator -> allocHookData is a pointer to our class in which we keep a per-document count over all all memory usage.  If the JSRuntime were per process, we couldn't keep independent counts unless we started using thread private data - perhaps you might argue that that is a better way to go.

As a final comment, can there be some documentation in jsapi.h or somewhere
about how such things ought to be done, so people can avoid this in future.

Comment 9

[Brendan Eich [:brendan]]

Peter: sorry, you are right -- the threadTPIndex is just never freed, and it should be a library static. Brian, can you fix this one?

JS API docs are on http://developer.mozilla.org/ now, and that's a wiki, so fixing errant docs is easy.

/be

Comment 10

[Wan-Teh Chang]

NSPR thread private indexes cannot be freed.  I don't know why.
Perhaps it's impossible to free a thread private index in a
thread-safe way.

Comment 11

[Brian Crowder]

Patch coming shortly

Comment 12

[Brian Crowder]

Attachment: threadTPIndex is a library-wide static

This probably isn't quite right.

Some assumptions made by this patch:
You can't have two different instances of libjs.so loaded (I think this is already the case, due to other statics), might be worth documenting somewhere
You must synchronize JS_NewRuntime() calls among threads on your own or risk having multiple threadTPIndex initializations happen -- this might not be TOO bad, unless you're trying to create hundreds of runtimes simultaneous (ill-advised)

Comment 13

[Brendan Eich [:brendan]]

Comment on attachment 251221 [details] [diff] [review]
threadTPIndex is a library-wide static


The single-threaded first runtime and first context requirements already exist, but need to be documented I'm sure.  Comment in jsapi.h?

> #ifdef JS_THREADSAFE
>+/*
>+ * The index for JSThread info, returned by PR_NewThreadPrivateIndex.
>+ * The value is visible and shared by all threads, but the data is
>+ * private to each thread.

Nit about pre-existing comment that you moved here: s/value is visible/index is visible/, as "value" and "data" are too similar -- or use "index value is visible".

>+ */
>+static PRUintn threadTPIndex;
>+static JSBool  tpIndexInited = JS_FALSE;
>+
>+PRStatus
>+js_InitThreadPrivateIndex(void *ptr)
>+{
>+    PRStatus status;
>+    if (tpIndexInited)
>+        return PR_SUCCESS;

Nit: blank line between decls and stmts as usual.

>+
>+    status = PR_NewThreadPrivateIndex(threadTPIndex, ptr);
>+
>+    if (status == PR_SUCCESS) {
>+        tpIndexInited = JS_TRUE;
>+    }

Nit: don't overbrace one-line then clauses.

> /*
>+ * Initalize a library-wide thread private data index, and remember that is has
>+ * already been done, so that it only happens once ever.
>+ */
>+extern int
>+js_InitThreadPrivateIndex(void *ptr);

Typos above ("Initialize" misspelled, "that it has" vs. "that is has", and put "only" after the verb for best usage. Thanks,

/be

Comment 14

[Brian Crowder]

Will address the nits shortly.  Peter, can you give this patch a try, please?

Comment 15

[Brian Crowder]

Attachment: tpindex fix, v2

Nits addressed.

Also fixed made the return value be "int" in the C file, to match the header.  Should these really be PRStatus things?  What header provides that?

Comment 16

[Wan-Teh Chang]

"prtypes.h" defines the PRStatus type.  You can use LXR to find this out.

Comment 17

[Brian Crowder]

I meant in the context of Spidermonkey.  I don't see that header included anywhere else in the library, except in the cpucfg module.  Certainly not in any header.

Comment 18

[Brendan Eich [:brendan]]

Comment on attachment 251223 [details] [diff] [review]
tpindex fix, v2

>-    if (PR_FAILURE == PR_NewThreadPrivateIndex(&rt->threadTPIndex,
>-                                               js_ThreadDestructorCB)) {
>+    if (PR_FAILURE == js_InitThreadPrivateIndex(js_ThreadDestructorCB)) {

Forgot to say: don't propagate PRStatus details out of jscntxt.c, rectify return type to JSBool.

> /*
>+ * The index for JSThread info, returned by PR_NewThreadPrivateIndex.  The
>+ * index value is visible and shared by all threads, but the data is private to
>+ * each thread.

Still fussing: "data associated with it is private...."

I'm sure I will r+ the next patch ;-).

/be

Comment 19

[Peter Naulls]

The software in question I have which uses this runs under a complex setup, which isn't presently in a state to work, and may not be for a few days, so I'm afraid I can't immediately try it.   I will as soon as I can if someone else hasn't.

Comment 20

[Brendan Eich [:brendan]]

(In reply to comment #17)
> I meant in the context of Spidermonkey.  I don't see that header included
> anywhere else in the library, except in the cpucfg module.  Certainly not in
> any header.

It's a bootleg include, nested in pratom.h, which is nested in jslock.h.  To be explicit, jscntxt.c (and only jscntxt.c) should #include "prtypes.h" only #ifdef JS_THREADSAFE.

/be

Comment 21

[Brian Crowder]

Attachment: tpindex fix, v2a

I don't blame you if you don't r+ this one; I added a comment to jsapi.h about the single-threaded runtime+cx creation; it's probably going to want tweaking.  The #include in jscntxt.c isn't necessary.  Only seems to matter if I put PRStatus in the header file.  Conversion to JSBool for this new routine solves that problem.

Comment 22

[Brendan Eich [:brendan]]

(In reply to comment #21)
> Created an attachment (id=251239) [details]
> tpindex fix, v2a
> 
> I don't blame you if you don't r+ this one; I added a comment to jsapi.h about
> the single-threaded runtime+cx creation; it's probably going to want tweaking.

Probably should save the real doc for MDC -- easier to tweak a wiki. I'll have a look in a sec.
 
> The #include in jscntxt.c isn't necessary.  Only seems to matter if I put
> PRStatus in the header file.  Conversion to JSBool for this new routine solves
> that problem.

Sure sure -- but my point was that jscntxt.c contains direct uses of PRStatus so it ought to directly include "prtypes.h", not count on the bootleg include via jslock.h/pratom.h.

/be

Comment 23

[Brendan Eich [:brendan]]

Comment on attachment 251239 [details] [diff] [review]
tpindex fix, v2a

>-    if (PR_FAILURE == PR_NewThreadPrivateIndex(&rt->threadTPIndex,
>-                                               js_ThreadDestructorCB)) {
>+    if (!js_InitThreadPrivateIndex(js_ThreadDestructorCB)) {
>         goto bad;
>     }

No braces for single-line then statement.

> /*
>  * Initialization, locking, contexts, and memory allocation.
>+ *
>+ * It is important that the first runtime and first context be created in a
>+ * single-threaded fashion, otherwise the behavior of the library is undefined.

This is fine -- maybe add a link to the MDC docs just below?

r=me with these changes.

/be

Comment 24

[Brian Crowder]

Attachment: tpindex fix, v2b

Tried adding an include for "prtypes.h", but Makefile.ref barfs (need more -I?).  If you still want that, I can do a follow-up or a separate bug.

Comment 25

[Brendan Eich [:brendan]]

(In reply to comment #24)
> Created an attachment (id=251249) [details]
> tpindex fix, v2b
> 
> Tried adding an include for "prtypes.h", but Makefile.ref barfs (need more
> -I?).

See comment 20 again.

/be

Comment 26

[Brian Crowder]

Attachment: fix v3

Ah, yes, the old JS_THREADSAFE-build-and-run-test-for-threaded-code-routine.  Very clever.

Comment 27

[Brian Crowder]

I will wait to land this until I know whether it resolves Peter's problem.

Comment 28

[Brendan Eich [:brendan]]

There's a /bona fide/ bug to fix, summarized correctly: all embeddings that make JSRuntimes more than once over the life of a process, esp. those using a runtime per thread, are likely to run out of TPIs.  The patch fixes that, so I say check it in ASAP, and if Peter runs into follow-on or unrelated but confounding issues, we can track 'em in other bugs.

/be

Comment 29

[Brian Crowder]

jsapi.c:   3.308
jsapi.h:   3.138
jscntxt.c: 3.104
jscntxt.h: 3.143