807480 - Separate per-thread fields used for debug GC root tracking out of the main JSRuntime*

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Niko Matsakis [:nmatsakis]]

Currently there are a number of global fields in JSRuntime* which are basically tracking per-thread state.  This makes sense on the current trunk since there is only ever a single thread associated with a runtime, but as Parallel JS (nee Rivertrail) starts to land this assumption no longer holds.

This patch makes a struct, currently called |JS::PerThreadData|, that stores per-thread data from the runtime.  There is one instance of this struct embedded in "Runtime" itself (the field |mainThread|).  For now I have only migrated the debug GC fields into |PerThread|, those are the ones causing me immediate pain.  Eventually more fields will want to move into there.

The eventual goal is to distinguish thread-safe code, which will take as argument a |JS::PerThread*|, from non-thread-safe code, which will take a |JSRuntime*| or |JSContext*|.

Comment 1

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to isolate GC fields.

Comment 2

[Luke Wagner [:luke]]

Comment on attachment 677423 [details] [diff] [review]
Patch to isolate GC fields.

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

I think you're right on with the idea of using PerThreadData as a static-type-y way of distinguishing the RT-accessible parts of the engine from the rest.  Couple nits and some questions below before ready to land:

::: js/src/jsapi.cpp
@@ +715,5 @@
>  
>  static const JSSecurityCallbacks NullSecurityCallbacks = { };
>  
> +JS::PerThreadData::PerThreadData(JSRuntime *runtime)
> +    : runtime_(runtime)

2-space indent before :

::: js/src/jscntxt.h
@@ +389,5 @@
>  
>  #define NAME_OFFSET(name)       offsetof(JSAtomState, name)
>  #define OFFSET_TO_NAME(rt,off)  (*(js::FixedHeapPtr<js::PropertyName>*)((char*)&(rt)->atomState + (off)))
>  
> +namespace JS {

namespace js, since this engine-internal

@@ +404,5 @@
> + * arguments instead of |JSContext*| or |JSRuntime*|.
> + */
> +struct PerThreadData
> +{
> +private:

We tend to use 'struct' to mean "POD, public data" and 'class' to mean "has privates we protect", so could you use 'class' and drop the 'private' ?

@@ +414,5 @@
> +     * functions like |associatedWith()| below.
> +     */
> +    JSRuntime *runtime_;
> +
> +public:

two space indent

@@ +437,5 @@
> +
> +    PerThreadData(JSRuntime *runtime);
> +
> +    bool associatedWith(const JSRuntime *rt) { return runtime_ == rt; }
> +    inline JSRuntime *runtime();

Given that "JSRuntime" as a type means "main thread", it seems scary to have the JSRuntime a single innocent-looking function-call away.

@@ +439,5 @@
> +
> +    bool associatedWith(const JSRuntime *rt) { return runtime_ == rt; }
> +    inline JSRuntime *runtime();
> +};
> +}

For closing }, we tend to write
  }  // namespace js

::: js/src/jscntxtinlines.h
@@ +589,5 @@
> +JS::PerThreadData::runtime()
> +{
> +    // Eventually, this function will assert that parallel threads are
> +    // not active. - NDM
> +    return runtime_;

Ahh, this answers my other comment.  To make this all super-explicit, could we:

  class PerThreadData
  {
    public:
      MainThreadData *asMainThreadData() const {
          JS_ASSERT(that we are on the main thread and no workers are active)
          return static_cast<MainThreadData *>(this);
      }
  };

  class MainThreadData : public PerThreadData
  {
    public:
      JSRuntime *runtime() const;
  }

?  (Or, if the StackIter change was the only use and we can avoid that, perhaps we don't need this at all.)

::: js/src/vm/Stack.cpp
@@ +1448,5 @@
> +    // the moment.
> +    if (other.maybecx_) {
> +        return other.maybecx_->runtime;
> +    }
> +    return TlsPerThreadData.get()->runtime();

I understand we are going to be incrementally pushing things into PerThreadData, but the StackIter changes seem a little too incoherent to land.  It seems like our root-tracking logic would be moved into PerThreadData which would avoid this stuff.

Comment 3

[Niko Matsakis [:nmatsakis]]

All the comments makes sense, I'll upload a new patch.  But I don't quite understand what you mean regarding the root-tracking logic.  I probably just don't understand what's going on there, I didn't read too deply into that. I'll look at it tomorrow and get back to you if I have any questions.

Comment 4

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to isolate GC fields.

Comment 5

[Niko Matsakis [:nmatsakis]]

Regarding the new patch: locally, I built with the exact root analysis and tested with GC Zeal set to 6.  The patch does not run all of jit-tests cleanly; however, I have the same set of failures both with and without the patch.

Comment 6

[Luke Wagner [:luke]]

Comment on attachment 678044 [details] [diff] [review]
Patch to isolate GC fields.

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

Great!

::: js/src/gc/Root.h
@@ +524,5 @@
>      {
>  #if defined(JSGC_ROOT_ANALYSIS) || defined(JSGC_USE_EXACT_ROOTING)
> +        PerThreadDataFriendFields *pt =
> +          const_cast<PerThreadDataFriendFields *>(PerThreadDataFriendFields::getMainThread(rtArg));
> +        commonInit(pt->thingGCRooters);

Can you either change PerThreadDataFriendFields::getMainThread to take/return a non-const pointer or, assuming it is necessary elsewhere, add an overload?  That would avoid this unsightly const_cast.  Ditto for 'get' below.

::: js/src/jscntxt.h
@@ +448,5 @@
> +     * parallel sections.  See definition of |PerThreadData| struct
> +     * above for more details.
> +     *
> +     * N.B.: This must appear FIRST to appease the hard-coded
> +     * calculations in |PerThreadDataFriendFields| */

Perhaps instead:

  NB: This field is statically asserted to be at offset sizeof(RuntimeFriendFields). See PerThreadDataFriendFields::getMainThread.

?  This assures the reader there is a static assert and also gives a bit more info as to why this is necessary.

Also, the closing */ goes on its own line.

::: js/src/jsgc.cpp
@@ +1125,5 @@
>  
>  #ifdef DEBUG
>      if (trc->runtime->gcIncrementalState == MARK_ROOTS)
> +        trc->runtime->mainThread.gcSavedRoots.append(
> +            js::PerThreadData::SavedGCRoot(thing, traceKind));

If I'm not mistaken you can get away from js:: prefixing here and in the next hunk.

::: js/src/jspubtd.h
@@ +320,5 @@
>  };
>  
> +class PerThreadData;
> +
> +struct PerThreadDataFriendFields {

{ on newline

@@ +335,5 @@
> +    static const PerThreadDataFriendFields *get(const js::PerThreadData *pt) {
> +        return reinterpret_cast<const PerThreadDataFriendFields *>(pt);
> +    }
> +
> +    static const PerThreadDataFriendFields *getMainThread(const JSRuntime *pt) {

'rt' instead of 'pt'

Comment 7

[Luke Wagner [:luke]]

(I should have said: r+ assuming these fixes)

Comment 8

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to isolate GC fields.

Comment 9

[Mozilla RelEng Bot]

Try run for baf51f2941b3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=baf51f2941b3
Results (out of 266 total builds):
    success: 241
    warnings: 19
    failure: 6
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-baf51f2941b3

Comment 10

[Mozilla RelEng Bot]

Try run for baf51f2941b3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=baf51f2941b3
Results (out of 272 total builds):
    success: 245
    warnings: 19
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-baf51f2941b3

Comment 11

[Niko Matsakis [:nmatsakis]]

Latest test run seems clean.

Comment 12

[Mozilla RelEng Bot]

Try run for baf51f2941b3 is complete.
Detailed breakdown of the results available here:
    https://tbpl.mozilla.org/?tree=Try&rev=baf51f2941b3
Results (out of 273 total builds):
    success: 246
    warnings: 19
    failure: 8
Builds (or logs if builds failed) available at:
http://ftp.mozilla.org/pub/mozilla.org/firefox/try-builds/[e-mail]-baf51f2941b3

Comment 13

[Niko Matsakis [:nmatsakis]]

Attachment: Patch to isolate GC fields.

Comment 14

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/651dc9d52259

Comment 15

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/651dc9d52259