786136 - Associate JSRuntime with a single thread

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

According to terrence's and my code research, there are already mechanisms in the engine to ensure that a JSRuntime is accessed by only a single thread at a time (see bug 650411), and that the engine is notified before a runtime is passed off from one thread to the other so that rt->ownerThread_ is kept up to date.

If we additionally made the restriction that a thread can use at most one runtime at a time, then we can maintain a pointer to the runtime in thread-local storage, and avoid the need to have either a JSRuntime* or JSContext* whenever using the JSAPI.

Comment 1

[Terrence Cole [:terrence]]

Attachment: v0

Well, lets give this a shot:
https://tbpl.mozilla.org/?tree=Try&rev=29acd14fc774

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 656099 [details] [diff] [review]
v0

Review of attachment 656099 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +722,5 @@
> +/*
> + * Thread Local Storage slot for storing the runtime for a thread.
> + */
> +#ifdef JS_THREADSAFE
> +static PRUintn js_RuntimeTLSIndex;

All the PR* integer types were recently changed to stdint types.  You should use |unsigned| here.

Comment 3

[Terrence Cole [:terrence]]

Attachment: v1: without nspr

Well, on more reflection, it doesn't make much sense to use NSPR here.  It's not as if we don't need stack roots in !JS_THREADSAFE builds :-).  I've got a non-nspr version working (except on MacOSX, because I can't spell).

New try run with corrected spelling up at:
https://tbpl.mozilla.org/?tree=Try&rev=5988be59cd17

Comment 4

[Terrence Cole [:terrence]]

WinXP is why we can't have nice things.

New try run up at:
https://tbpl.mozilla.org/?tree=Try&rev=fb4288a9b2dd

Comment 5

[Terrence Cole [:terrence]]

Attachment: v0

So, it turns out that constructs of the form |Foo foo = init;| are required to be able to call the copy constructor.  C++11 has initializer lists: these still require a copy constructor for |Foo foo = {init};|, but not for |Foo foo {init}|, oddly.  Overall, this is probably not enough of a clarity win to justify a change.

That said, I think we still want to provide new non-cx and non-rt constructors for the browser to use (and for cases where we only have a maybecx).  The good news is that since there is little performance concern we can just use MFBT's proven TLS class.

https://tbpl.mozilla.org/?tree=Try&rev=2cbd2f617fc1

Comment 6

[Terrence Cole [:terrence]]

Attachment: v1 - part2 Rooted changes.

Comment 7

[Terrence Cole [:terrence]]

Attachment: v1: part1 - Implement JSRuntime in TLS and assert that this actually works.

Comment 8

[Luke Wagner [:luke]]

Comment on attachment 656963 [details] [diff] [review]
v1: part1 - Implement JSRuntime in TLS and assert that this actually works.

Review of attachment 656963 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/jsapi.cpp
@@ +1024,2 @@
>      ownerThread_ = (void *)0xc1ea12;  /* "clear" */
> +    JS::TlsRuntime.set((JSRuntime *)0xc1ea12);

I don't think it makes sense to set this to "clear" here: we are freeing up this thread's TLS slot so that any other runtime can use it; this isn't an invalid state (unlike the "clear" state of a particular JSRuntime's ownerThread_).  Let's just set it to NULL.

Could you also add a call to clearOwnerThread in ~JSRuntime (replacing the JS_ASSERT(onOwnerThread()) so that way destroying the JSRuntime leaves the TLS slot open for future runtimes?

@@ +1034,5 @@
>  JS_FRIEND_API(bool)
>  JSRuntime::onOwnerThread() const
>  {
> +    return ownerThread_ == PR_GetCurrentThread() &&
> +           this == JS::TlsRuntime.get();

The predicate doesn't seem to match the name onOwnerThread anymore.  It's also not great to have a single assert asserting two things (so that when there is a failure you don't know which conjunct failed).  How about s/onOwnerThread/assertValidThread/ (where the latter does the asserting in the body and returns 'void'.

::: js/src/jsapi.h
@@ +14,5 @@
>  #include "mozilla/Attributes.h"
>  #include "mozilla/FloatingPoint.h"
>  #include "mozilla/StandardInteger.h"
> +#ifdef __cplusplus
> +#  include "mozilla/ThreadLocal.h"

single-space indent

Comment 9

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 656961 [details] [diff] [review]
v1 - part2 Rooted changes.

Review of attachment 656961 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Root.h
@@ +280,5 @@
>          JS_ASSERT(!RootMethods<T>::poisoned(ptr));
>  #endif
>      }
>  
> +    void init(JSRuntime *rt_)

I guess this should be rtArg.

Comment 10

[Terrence Cole [:terrence]]

Green (modulo busted infra) try run at:
https://tbpl.mozilla.org/?tree=Try&rev=2cbd2f617fc1

Pushed at:
https://hg.mozilla.org/integration/mozilla-inbound/rev/4e1ac73d8a2d

Comment 11

[Brendan Eich [:brendan]]

/Users/brendaneich/Hacking/hg.mozilla.org/integration/mozilla-inbound/js/src/jsapi.cpp:1085:20: warning: initialization of pointer of type 'JSRuntime *' to null from a constant boolean expression [-Wbool-conversions]
            return false;
                   ^~~~~
1 warning generated.

grep warning is your friend. r=me on fix.

/be

Comment 12

[Philipp von Weitershausen [:philikon]]

https://hg.mozilla.org/mozilla-central/rev/4e1ac73d8a2d

Comment 13

[Terrence Cole [:terrence]]

Attachment: fix

Very odd!  G++ is not warning for me here.  Will have to go dig into this.  Thanks for the catch.

https://hg.mozilla.org/integration/mozilla-inbound/rev/82e8e2679938

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/82e8e2679938