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)
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•12 years ago
|
||
Attachment #677423 -
Flags: review?(luke)
![]() |
||
Comment 2•12 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•12 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•12 years ago
|
||
Attachment #677423 -
Attachment is obsolete: true
Attachment #678044 -
Flags: review?(luke)
Reporter | ||
Comment 5•12 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•12 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•12 years ago
|
||
(I should have said: r+ assuming these fixes)
Reporter | ||
Comment 8•12 years ago
|
||
Attachment #678044 -
Attachment is obsolete: true
Comment 9•12 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•12 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•12 years ago
|
||
Latest test run seems clean.
Comment 12•12 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•12 years ago
|
||
Attachment #678632 -
Attachment is obsolete: true
Reporter | ||
Updated•12 years ago
|
Keywords: checkin-needed
Comment 14•12 years ago
|
||
Keywords: checkin-needed
Comment 15•12 years ago
|
||
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.
Description
•