Closed Bug 365048 Opened 14 years ago Closed 14 years ago

JSRuntime.threadTPIndex should be a static

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED

People

(Reporter: peter, Assigned: crowderbt)

References

Details

Attachments

(1 file, 4 obsolete files)

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
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.
Status: UNCONFIRMED → RESOLVED
Closed: 14 years ago
Resolution: --- → WONTFIX
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.
Status: RESOLVED → UNCONFIRMED
Component: NSPR → JavaScript Engine
Product: NSPR → Core
Resolution: WONTFIX → ---
Version: other → Trunk
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/
Assignee: wtchang → general
QA Contact: nspr → general
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
Status: UNCONFIRMED → NEW
Ever confirmed: true
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. 
Why do you need a separate JSRuntime for each document? Why can't you just keep the contexts separate?
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
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.
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
Assignee: general → crowder
Summary: _PR_TPD_LIMIT is pointlessly limited to 128 → JSRuntime.threadTPIndex should be a static, freed on JS_ShutDown
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.
Summary: JSRuntime.threadTPIndex should be a static, freed on JS_ShutDown → JSRuntime.threadTPIndex should be a static
Patch coming shortly
Status: NEW → ASSIGNED
OS: Linux → All
Hardware: PC → All
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)
Attachment #251221 - Flags: review?(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
Will address the nits shortly.  Peter, can you give this patch a try, please?
Attached patch tpindex fix, v2 (obsolete) — Splinter Review
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?
Attachment #251221 - Attachment is obsolete: true
Attachment #251223 - Flags: review?(brendan)
Attachment #251221 - Flags: review?(brendan)
"prtypes.h" defines the PRStatus type.  You can use LXR to find this out.
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 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
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.
OS: All → Linux
Hardware: All → PC
(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
OS: Linux → All
Hardware: PC → All
Attached patch tpindex fix, v2a (obsolete) — Splinter Review
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.
Attachment #251223 - Attachment is obsolete: true
Attachment #251239 - Flags: review?(brendan)
Attachment #251223 - Flags: review?(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 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
Attachment #251239 - Flags: review?(brendan) → review+
Attached patch tpindex fix, v2b (obsolete) — Splinter Review
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.
Attachment #251239 - Attachment is obsolete: true
Attachment #251249 - Flags: review+
(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
Attached patch fix v3Splinter Review
Ah, yes, the old JS_THREADSAFE-build-and-run-test-for-threaded-code-routine.  Very clever.
Attachment #251249 - Attachment is obsolete: true
Attachment #251261 - Flags: review?(brendan)
Attachment #251261 - Flags: review?(brendan) → review+
I will wait to land this until I know whether it resolves Peter's problem.
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
jsapi.c:   3.308
jsapi.h:   3.138
jscntxt.c: 3.104
jscntxt.h: 3.143
Status: ASSIGNED → RESOLVED
Closed: 14 years ago14 years ago
Resolution: --- → FIXED
Flags: in-testsuite-
Depends on: 378918
You need to log in before you can comment on or make changes to this bug.