Closed Bug 1135100 Opened 9 years ago Closed 9 years ago

Compacting GC data races reported during parallel updates

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla39
Tracking Status
firefox39 --- fixed

People

(Reporter: jonco, Assigned: jonco)

References

Details

Attachments

(3 files)

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).
Part 1 - when marking an Id or Value, don't update it if the pointer hasn't changed.
Attachment #8567846 - Flags: review?(terrence)
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)
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 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 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+
Attachment #8567848 - Flags: review?(terrence) → review+
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.
(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
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.
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)
(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.