Closed
Bug 1135100
Opened 9 years ago
Closed 9 years ago
Compacting GC data races reported during parallel updates
Categories
(Core :: JavaScript: GC, defect)
Core
JavaScript: GC
Tracking
()
RESOLVED
FIXED
mozilla39
Tracking | Status | |
---|---|---|
firefox39 | --- | fixed |
People
(Reporter: jonco, Assigned: jonco)
References
Details
Attachments
(3 files)
2.54 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
3.00 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
10.97 KB,
patch
|
terrence
:
review+
|
Details | Diff | Splinter Review |
There are three conflicts reported by TSAN so I'm making a single bug to address the issue, which is that IsForwarded(thing) reads from a location in a cell which may be independently updated. The read is done to check for the magic value that means the cell is relocated. Currently the updates are always to the same value since the pointer types store in this location are to things which we don't relocate at present (if we did relocate them they would be updates from a non-magic value to another non-magic value).
Assignee | ||
Comment 1•9 years ago
|
||
Part 1 - when marking an Id or Value, don't update it if the pointer hasn't changed.
Attachment #8567846 -
Flags: review?(terrence)
Assignee | ||
Comment 2•9 years ago
|
||
Part 2 - use the pointer type in IsForwarded() to ignore pointers to types that we know we don't move.
Attachment #8567847 -
Flags: review?(terrence)
Assignee | ||
Comment 3•9 years ago
|
||
Part 2.5 - remove callers of IsForwarded(Cell*), fixing a bug in the cross compartment wrapper map postbarrier where we could reinterpret_cast a JSScript* to a JSObject* (my bad).
Attachment #8567848 -
Flags: review?(terrence)
Comment 4•9 years ago
|
||
Comment on attachment 8567846 [details] [diff] [review] bug1135100-dont-update-unchanged-pointers Review of attachment 8567846 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/gc/Marking.cpp @@ -754,5 @@ > if (v->isMarkable()) { > MOZ_ASSERT(v->toGCThing()); > void *thing = v->toGCThing(); > trc->setTracingLocation((void *)v); > - MarkKind(trc, &thing, v->gcKind()); \o/
Attachment #8567846 -
Flags: review?(terrence) → review+
Comment 5•9 years ago
|
||
Comment on attachment 8567847 [details] [diff] [review] bug1135100-use-type-in-forwarded-check Review of attachment 8567847 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/jsgc.h @@ +1201,5 @@ > +TYPE_MIGHT_BE_FORWARDED(Shape, false) > +TYPE_MIGHT_BE_FORWARDED(BaseShape, false) > +TYPE_MIGHT_BE_FORWARDED(jit::JitCode, false) > +TYPE_MIGHT_BE_FORWARDED(LazyScript, false) > +TYPE_MIGHT_BE_FORWARDED(ObjectGroup, false) I'm currently working on a generic FOR_EACH_GC_TYPE macro/table that should simplify any defs of this form. Thus, could we make this explicitly a data definition under the assumption that it will be less ugly later? Something like MapTypeToFinalizeKind: template <typename T> struct MightBeForwarded {}; template <> struct MightBeForwarded<JSObject> { static const bool value = true; }; template <> struct MightBeForwarded<JSScript> { static const bool value = false; }; template <> struct MightBeForwarded<LazyScript> { static const bool value = false; }; template <> struct MightBeForwarded<Shape> { static const bool value = false; }; ....
Attachment #8567847 -
Flags: review?(terrence) → review+
Updated•9 years ago
|
Attachment #8567848 -
Flags: review?(terrence) → review+
Comment 6•9 years ago
|
||
Actually, after discussing on IRC, we want to go with the dynamicish dispatch because that will take into account derivation -- which templates cannot do -- to allow us to only specify rows for the actual storage types.
Assignee | ||
Comment 7•9 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/24ab282d52d2 https://hg.mozilla.org/integration/mozilla-inbound/rev/2dd5b0353265 https://hg.mozilla.org/integration/mozilla-inbound/rev/8cdd8707edc6
https://hg.mozilla.org/mozilla-central/rev/8cdd8707edc6 https://hg.mozilla.org/mozilla-central/rev/2dd5b0353265 https://hg.mozilla.org/mozilla-central/rev/24ab282d52d2
Status: NEW → RESOLVED
Closed: 9 years ago
status-firefox39:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla39
Comment 9•9 years ago
|
||
(In reply to Terrence Cole [:terrence] from comment #6) > Actually, after discussing on IRC, we want to go with the dynamicish > dispatch because that will take into account derivation -- which templates > cannot do -- to allow us to only specify rows for the actual storage types. O ye of little faith. ////////////////////////////////////////////////////////////////////// #include <stdio.h> #include <type_traits> template <typename T, typename U> struct IsBaseOf { static const bool value = std::is_base_of<T, U>::value; }; struct JSObject {}; struct JSScript {}; struct LazyScript {}; struct Shape {}; struct PlainObject : JSObject {}; struct JSFunction : JSObject {}; struct NonObject {}; enum class Objectivity { NonObject, ObjectOrSubclass }; template <typename T, Objectivity = IsBaseOf<JSObject, T>::value ? Objectivity::ObjectOrSubclass : Objectivity::NonObject> struct MightBeForwarded; template <typename T> struct MightBeForwarded<T, Objectivity::ObjectOrSubclass> { static const int value = 1; }; template <typename T> struct MightBeForwarded<T, Objectivity::NonObject> { static const int value = 2; }; template <> struct MightBeForwarded<JSScript> { static const int value = 3; }; template <> struct MightBeForwarded<LazyScript> { static const int value = 4; }; template <> struct MightBeForwarded<Shape> { static const int value = 5; }; int main() { printf("object: %d\n", MightBeForwarded<JSObject>::value); printf("plain object: %d\n", MightBeForwarded<PlainObject>::value); printf("function: %d\n", MightBeForwarded<JSFunction>::value); printf("non-object: %d\n", MightBeForwarded<NonObject>::value); printf("script: %d\n", MightBeForwarded<JSScript>::value); printf("lazy script: %d\n", MightBeForwarded<LazyScript>::value); printf("shape: %d\n", MightBeForwarded<Shape>::value); } ////////////////////////////////////////////////////////////////////// [jwalden@find-waldo-now tmp]$ \ clang++-tip -std=c++11 subclasses.cpp -o subclasses && ./subclasses object: 1 plain object: 1 function: 1 non-object: 2 script: 3 lazy script: 4 shape: 5
Assignee | ||
Comment 10•9 years ago
|
||
Ah neat, I forgot about IsBaseOf. We don't event need a template function if we use this. Filed bug 1136768 to tidy this up.
Assignee | ||
Comment 11•9 years ago
|
||
Nathan, I don't know how often the TSAN tests get run, but this should have fixed the issues in bug 1131711, bug 1131714 and bug 1132170. Can you confirm and then we'll can close those bugs?
Flags: needinfo?(nfroyd)
Comment 12•9 years ago
|
||
(In reply to Jon Coppeard (:jonco) from comment #11) > Nathan, I don't know how often the TSAN tests get run, but this should have > fixed the issues in bug 1131711, bug 1131714 and bug 1132170. Can you > confirm and then we'll can close those bugs? I haven't seen these recently, so I think we're good to go. I'll refile if I see problems. Thanks!
Flags: needinfo?(nfroyd)
You need to log in
before you can comment on or make changes to this bug.
Description
•