Closed Bug 807480 Opened 12 years ago Closed 12 years ago

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

Categories

(Core :: JavaScript Engine, defect)

x86
macOS
defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla19

People

(Reporter: nmatsakis, Unassigned)

References

Details

Attachments

(1 file, 3 obsolete files)

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*|.
Attached patch Patch to isolate GC fields. (obsolete) — Splinter Review
Attachment #677423 - Flags: review?(luke)
Blocks: PJS
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)
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.
Attached patch Patch to isolate GC fields. (obsolete) — Splinter Review
Attachment #677423 - Attachment is obsolete: true
Attachment #678044 - Flags: review?(luke)
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 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+
(I should have said: r+ assuming these fixes)
Attached patch Patch to isolate GC fields. (obsolete) — Splinter Review
Attachment #678044 - Attachment is obsolete: true
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
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
Latest test run seems clean.
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
Attachment #678632 - Attachment is obsolete: true
Keywords: checkin-needed
Status: NEW → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla19
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: