Closed
Bug 807480
Opened 11 years ago
Closed 11 years ago
Separate per-thread fields used for debug GC root tracking out of the main JSRuntime*
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla19
People
(Reporter: nmatsakis, Unassigned)
References
Details
Attachments
(1 file, 3 obsolete files)
24.25 KB,
patch
|
Details | Diff | Splinter Review |
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•11 years ago
|
||
Attachment #677423 -
Flags: review?(luke)
![]() |
||
Comment 2•11 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•11 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•11 years ago
|
||
Attachment #677423 -
Attachment is obsolete: true
Attachment #678044 -
Flags: review?(luke)
Reporter | ||
Comment 5•11 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•11 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•11 years ago
|
||
(I should have said: r+ assuming these fixes)
Reporter | ||
Comment 8•11 years ago
|
||
Attachment #678044 -
Attachment is obsolete: true
Comment 9•11 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•11 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•11 years ago
|
||
Latest test run seems clean.
Comment 12•11 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•11 years ago
|
||
Attachment #678632 -
Attachment is obsolete: true
Reporter | ||
Updated•11 years ago
|
Keywords: checkin-needed
Comment 14•11 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/651dc9d52259
Keywords: checkin-needed
Comment 15•11 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/651dc9d52259
Status: NEW → RESOLVED
Closed: 11 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in
before you can comment on or make changes to this bug.
Description
•