Closed Bug 786136 Opened 8 years ago Closed 8 years ago

Associate JSRuntime with a single thread


(Core :: JavaScript Engine, defect)

Not set





(Reporter: sfink, Assigned: terrence)




(3 files, 3 obsolete files)

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.
Assignee: general → terrence
Blocks: ExactRooting
OS: Linux → All
Hardware: x86_64 → All
Attached patch v0 (obsolete) — Splinter Review
Well, lets give this a shot:
Comment on attachment 656099 [details] [diff] [review]

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

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

All the PR* integer types were recently changed to stdint types.  You should use |unsigned| here.
Attached patch v1: without nspr (obsolete) — Splinter Review
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:
Attachment #656099 - Attachment is obsolete: true
WinXP is why we can't have nice things.

New try run up at:
Attached patch v0 (obsolete) — Splinter Review
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.
Attachment #656284 - Attachment is obsolete: true
Attachment #656662 - Attachment is obsolete: true
Attachment #656961 - Flags: review?(wmccloskey)
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
Attachment #656963 - Flags: review?(luke) → review+
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.
Attachment #656961 - Flags: review?(wmccloskey) → review+
/Users/brendaneich/Hacking/ 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.

Closed: 8 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
Attached patch fixSplinter Review
Very odd!  G++ is not warning for me here.  Will have to go dig into this.  Thanks for the catch.
Attachment #657369 - Flags: review+
Depends on: 789552
You need to log in before you can comment on or make changes to this bug.