GC: make IsAboutToBeFinalized indirect

RESOLVED FIXED in mozilla15

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: terrence, Assigned: terrence)

Tracking

Trunk
mozilla15
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(2 attachments, 3 obsolete attachments)

(Assignee)

Description

5 years ago
In some cases we only mark through an object conditionally on IsAboutToBeFinalized (e.g. !IsMarked).  In order to move an object, we need to see all of the properties in things that may references it, even if we would not normally see them through Mark because they are IsAboutToBeFinalized.  In these cases, IsAboutToBeFinalized may need to move the object itself if it would return true.
(In reply to Terrence Cole [:terrence] from comment #0)
> In these cases, IsAboutToBeFinalized may need to move
> the object itself if it would return true.

If it would return false, I think you mean.
(Assignee)

Comment 2

5 years ago
Created attachment 606408 [details] [diff] [review]
v0

I managed to make this less ugly by adding a type specialized args version of IsAboutToBeFinalized for each type we use this on.  I would have added Object/Script/Type/etc to the name like we have for the MarkFoo functions, but the name is already absurdly long.  If you'd like I'll gladly re-spin these as !IsMarkedFoo, IsNotMarkedFoo, or whatever you think is best.

Try run is up at:
https://tbpl.mozilla.org/?tree=Try&rev=caf66c697613
Attachment #606408 - Flags: review?(wmccloskey)
(Assignee)

Comment 3

5 years ago
On further reflection, I think the thing that bothers me about the name is that Is...() should not be effectful.  Perhaps GetMarkStatus() or somesuch would be more appropriate?
(In reply to Terrence Cole [:terrence] from comment #3)
> On further reflection, I think the thing that bothers me about the name is
> that Is...() should not be effectful.  Perhaps GetMarkStatus() or somesuch
> would be more appropriate?

That doesn't tell you what the return value means (does true mean marked or unmarked?). How about CheckObjectMarked, CheckScriptMarked, etc. We'll have to invert all the tests, but I think that's more intuitive anyway. Also, can you move them to the js::gc namespace?
(Assignee)

Comment 5

5 years ago
Perhaps it would make sense to move them in jsgcmark as well?
(In reply to Terrence Cole [:terrence] from comment #5)
> Perhaps it would make sense to move them in jsgcmark as well?

Yeah, that sounds good.
(Assignee)

Updated

5 years ago
Attachment #606408 - Flags: review?(wmccloskey)
(Assignee)

Updated

5 years ago
Depends on: 753479
(Assignee)

Comment 7

5 years ago
Created attachment 622600 [details] [diff] [review]
v3: Several internal revisions later.

I think this new interface works well in practice.

The individual weakish mark/sweep functions are still extremely subtle to get right, however, so I would appreciate a particularly close review of those bits.
Attachment #606408 - Attachment is obsolete: true
Attachment #622600 - Flags: review?(wmccloskey)
(Assignee)

Comment 8

5 years ago
Created attachment 622987 [details] [diff] [review]
v4: 3 tiny changes.

1) This version removes IsMarked(Value*) -- I'm not sure how it snuck in, but IsMarked(EncapsulatedValue*) is all that is needed.
2) IsMarked(HeapValue*) -> IsMarked(EncapsulatedValue*), now that this type is available.
3) IsMarked(EncapsulatedValue*) lost its check for v->isMarkable(), which is absolutely needed by WeakMap.
Attachment #622600 - Attachment is obsolete: true
Attachment #622987 - Flags: review?(wmccloskey)
Attachment #622600 - Flags: review?(wmccloskey)
Comment on attachment 622987 [details] [diff] [review]
v4: 3 tiny changes.

Review of attachment 622987 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/src/gc/Marking.cpp
@@ +181,5 @@
> +    JS_ASSERT(thingp);
> +    JS_ASSERT(*thingp);
> +    if (!static_cast<Cell *>(*thingp)->compartment()->isCollecting())
> +        return true;
> +    return static_cast<Cell *>(*thingp)->isMarked();

I think you can take out these static_casts now that this is templated.

@@ +407,5 @@
> +        v->setString(str);
> +    } else {
> +        JSObject *obj = (JSObject *)v->toGCThing();
> +        rv = IsMarked<JSObject>(&obj);
> +        v->setObjectOrNull(obj);

Please use v->setObject. I can't imagine us ever changing a field from non-null to null.

::: js/src/jsatom.cpp
@@ +285,1 @@
>              continue;

Can you take out this continue statement? I think we still want to re-key in this case.

@@ +289,3 @@
>              e.removeFront();
> +        else
> +            e.rekeyFront(AtomHasher::Lookup(atom), entry);

Can you just pass |AtomStateEntry(atom, entry.isTagged())| for the second argument and remove the assignment to |entry| above? It seems a little cleaner to me.

::: js/src/jscompartment.cpp
@@ +442,5 @@
>  {
>      /* Remove dead wrappers from the table. */
>      for (WrapperMap::Enum e(crossCompartmentWrappers); !e.empty(); e.popFront()) {
> +        Value tmpKey = e.front().key;
> +        Value tmpVal = e.front().value;

Can you call these key and val instead of tmpKey and tmpVal?

@@ +445,5 @@
> +        Value tmpKey = e.front().key;
> +        Value tmpVal = e.front().value;
> +        bool keyMarked = IsValueMarked(&tmpKey);
> +        bool valMarked = IsValueMarked(&tmpVal);
> +        e.front().value = tmpVal;

In fact, can you just call IsValueMarked(&e.front().value) and remove tmpVal entirely?

@@ +447,5 @@
> +        bool keyMarked = IsValueMarked(&tmpKey);
> +        bool valMarked = IsValueMarked(&tmpVal);
> +        e.front().value = tmpVal;
> +        JS_ASSERT_IF(!keyMarked && valMarked, tmpKey.isString());
> +        if (!keyMarked && !valMarked)

This should be |!keyMarked || !valMarked|.

::: js/src/jsinfer.cpp
@@ +5948,5 @@
>              const ObjectTableEntry &entry = e.front().value;
>              JS_ASSERT(entry.object->proto == key.proto);
>  
>              bool remove = false;
>              if (!entry.object->isMarked())

It might be nice if you fix this one, although it's not needed for generational GC.

@@ +5955,5 @@
>                  if (JSID_IS_STRING(key.ids[i])) {
> +                    /*
> +                     * It is safe here to update the key without rekeying the
> +                     * hash table because the lookup is on the owner object,
> +                     * not the key.

I don't think this is true. The hash function is partly based on |obj->lastProperty()->propid().get()|, which is one of the ids that is being updated here. So in theory a re-key is necessary. In practice, we don't seem to deal with non-string IDs here. So they're all atoms, and unlikely to be relocated. So it's okay with me if you just want to assert that the key never moves. But please change the comment.

@@ +5983,4 @@
>              TypeObject *object = e.front().value;
>  
> +            bool keyMarked = IsScriptMarked(&key.script);
> +            if (!keyMarked || !object->isMarked())

Could you fix this isMarked as well? Should be easy.

::: js/src/jsscope.cpp
@@ +1406,5 @@
>      if (initialShapes.initialized()) {
>          for (InitialShapeSet::Enum e(initialShapes); !e.empty(); e.popFront()) {
>              const InitialShapeEntry &entry = e.front();
> +            Shape *tmpShape = entry.shape;
> +            JSObject *tmpProto = entry.proto;

Can you remove tmp from the names of these as well?

@@ +1412,5 @@
>                  e.removeFront();
> +            } else {
> +                JSObject *parent = tmpShape->getObjectParent();
> +                JS_ALWAYS_TRUE(!parent || IsObjectMarked(&parent));
> +                JS_ALWAYS_TRUE(parent == tmpShape->getObjectParent());

JS_ALWAYS_TRUE is only needed if you want to execute this for side effects in opt builds. So I think that both of these can be JS_ASSERT, since we don't expect parent to move.

@@ +1415,5 @@
> +                JS_ALWAYS_TRUE(!parent || IsObjectMarked(&parent));
> +                JS_ALWAYS_TRUE(parent == tmpShape->getObjectParent());
> +                InitialShapeEntry newKey;
> +                newKey.shape = ReadBarriered<Shape>(tmpShape);
> +                newKey.proto = tmpProto;

Could you add a constructor for InitialShapeEntry? Also, you don't need the ReadBarriered<Shape> coercion. It will happen automatically.

::: js/src/jswatchpoint.cpp
@@ +173,5 @@
>  bool
>  WatchpointMap::markIteratively(JSTracer *trc)
>  {
>      bool marked = false;
> +    for (Map::Enum iter(map); !iter.empty(); iter.popFront()) {

We usually call Enums |e|, so let's stick with that.

@@ +174,5 @@
>  WatchpointMap::markIteratively(JSTracer *trc)
>  {
>      bool marked = false;
> +    for (Map::Enum iter(map); !iter.empty(); iter.popFront()) {
> +        Map::Entry &e = iter.front();

Then this could be |entry|.

@@ +204,5 @@
>  void
>  WatchpointMap::markAll(JSTracer *trc)
>  {
> +    for (Map::Enum iter(map); !iter.empty(); iter.popFront()) {
> +        Map::Entry &e = iter.front();

Same changes here.

@@ +206,5 @@
>  {
> +    for (Map::Enum iter(map); !iter.empty(); iter.popFront()) {
> +        Map::Entry &e = iter.front();
> +        JSObject *tmpObj = e.key.object;
> +        jsid tmpId = e.key.id;

Maybe call them keyObj and keyId again.

@@ +232,5 @@
>  WatchpointMap::sweep()
>  {
>      for (Map::Enum r(map); !r.empty(); r.popFront()) {
>          Map::Entry &e = r.front();
> +        HeapPtrObject tmpObj(e.key.object);

Just call it obj?

::: js/src/jsweakmap.h
@@ +234,3 @@
>                  e.removeFront();
> +            else
> +                e.rekeyFront(k);

I don't think we'll ever need to rekey here. Can you just assert that |k == e.front().key|?

@@ +242,5 @@
>           * known-live part of the graph.
>           */
>          for (Range r = Base::all(); !r.empty(); r.popFront()) {
> +            JS_ASSERT(gc::IsMarked(&const_cast<Key &>(r.front().key)));
> +            JS_ASSERT(gc::IsMarked(&r.front().value));

Can you copy these to temporaries and then assert that they haven't moved?

::: js/src/vm/Debugger.cpp
@@ +1349,5 @@
>       */
>      JSRuntime *rt = trc->runtime;
>      for (CompartmentsIter c(rt); !c.done(); c.next()) {
> +        GlobalObjectSet &debuggees = c->getDebuggees();
> +        for (GlobalObjectSet::Enum iter(debuggees); !iter.empty(); iter.popFront()) {

Also |e|, please.
Attachment #622987 - Flags: review?(wmccloskey)
(Assignee)

Comment 10

5 years ago
Created attachment 624612 [details] [diff] [review]
Diff from v4-v5, since the changes are fairly extensive.

This might help with the second review.
(Assignee)

Comment 11

5 years ago
Created attachment 624613 [details] [diff] [review]
v5: With review comments addressed.
Attachment #622987 - Attachment is obsolete: true
Attachment #624613 - Flags: review?(wmccloskey)
Comment on attachment 624613 [details] [diff] [review]
v5: With review comments addressed.

Thanks.
Attachment #624613 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 13

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/502d5e87aeff
https://hg.mozilla.org/mozilla-central/rev/502d5e87aeff
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla15
You need to log in before you can comment on or make changes to this bug.