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

RESOLVED FIXED in mozilla19

Status

()

RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: nmatsakis, Unassigned)

Tracking

Trunk
mozilla19
x86
Mac OS X
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Reporter)

Description

6 years ago
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*|.
(Reporter)

Comment 1

6 years ago
Created attachment 677423 [details] [diff] [review]
Patch to isolate GC fields.
Attachment #677423 - Flags: review?(luke)
(Reporter)

Updated

6 years ago
Blocks: 801869

Comment 2

6 years ago
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.
Attachment #677423 - Flags: review?(luke)
(Reporter)

Comment 3

6 years ago
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.
(Reporter)

Comment 4

6 years ago
Created attachment 678044 [details] [diff] [review]
Patch to isolate GC fields.
Attachment #677423 - Attachment is obsolete: true
Attachment #678044 - Flags: review?(luke)
(Reporter)

Comment 5

6 years ago
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

6 years ago
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'
Attachment #678044 - Flags: review?(luke) → review+

Comment 7

6 years ago
(I should have said: r+ assuming these fixes)
(Reporter)

Comment 8

6 years ago
Created attachment 678632 [details] [diff] [review]
Patch to isolate GC fields.
Attachment #678044 - Attachment is obsolete: true

Comment 9

6 years ago
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/nmatsakis@mozilla.com-baf51f2941b3

Comment 10

6 years ago
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/nmatsakis@mozilla.com-baf51f2941b3
(Reporter)

Comment 11

6 years ago
Latest test run seems clean.

Comment 12

6 years ago
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/nmatsakis@mozilla.com-baf51f2941b3
(Reporter)

Comment 13

6 years ago
Created attachment 678958 [details] [diff] [review]
Patch to isolate GC fields.
Attachment #678632 - Attachment is obsolete: true
(Reporter)

Updated

6 years ago
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/651dc9d52259
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.