Last Comment Bug 730933 - GC: make IsAboutToBeFinalized indirect
: GC: make IsAboutToBeFinalized indirect
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla15
Assigned To: Terrence Cole [:terrence]
:
Mentors:
Depends on: 753479
Blocks: 721557
  Show dependency treegraph
 
Reported: 2012-02-27 12:41 PST by Terrence Cole [:terrence]
Modified: 2012-05-24 09:35 PDT (History)
2 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
v0 (26.68 KB, patch)
2012-03-15 17:17 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v3: Several internal revisions later. (32.48 KB, patch)
2012-05-09 18:29 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v4: 3 tiny changes. (32.46 KB, patch)
2012-05-10 17:36 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
Diff from v4-v5, since the changes are fairly extensive. (19.79 KB, patch)
2012-05-16 17:57 PDT, Terrence Cole [:terrence]
no flags Details | Diff | Splinter Review
v5: With review comments addressed. (36.94 KB, patch)
2012-05-16 17:58 PDT, Terrence Cole [:terrence]
wmccloskey: review+
Details | Diff | Splinter Review

Description Terrence Cole [:terrence] 2012-02-27 12:41:42 PST
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.
Comment 1 Bill McCloskey (:billm) 2012-02-27 12:43:04 PST
(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.
Comment 2 Terrence Cole [:terrence] 2012-03-15 17:17:02 PDT
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
Comment 3 Terrence Cole [:terrence] 2012-03-16 10:15:15 PDT
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?
Comment 4 Bill McCloskey (:billm) 2012-03-16 10:44:18 PDT
(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?
Comment 5 Terrence Cole [:terrence] 2012-03-16 10:53:51 PDT
Perhaps it would make sense to move them in jsgcmark as well?
Comment 6 Bill McCloskey (:billm) 2012-03-16 10:58:25 PDT
(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.
Comment 7 Terrence Cole [:terrence] 2012-05-09 18:29:32 PDT
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.
Comment 8 Terrence Cole [:terrence] 2012-05-10 17:36:43 PDT
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.
Comment 9 Bill McCloskey (:billm) 2012-05-16 15:56:23 PDT
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.
Comment 10 Terrence Cole [:terrence] 2012-05-16 17:57:30 PDT
Created attachment 624612 [details] [diff] [review]
Diff from v4-v5, since the changes are fairly extensive.

This might help with the second review.
Comment 11 Terrence Cole [:terrence] 2012-05-16 17:58:13 PDT
Created attachment 624613 [details] [diff] [review]
v5: With review comments addressed.
Comment 12 Bill McCloskey (:billm) 2012-05-21 18:24:07 PDT
Comment on attachment 624613 [details] [diff] [review]
v5: With review comments addressed.

Thanks.
Comment 13 Terrence Cole [:terrence] 2012-05-23 10:37:14 PDT
https://hg.mozilla.org/integration/mozilla-inbound/rev/502d5e87aeff
Comment 14 Ed Morley [:emorley] 2012-05-24 09:35:25 PDT
https://hg.mozilla.org/mozilla-central/rev/502d5e87aeff

Note You need to log in before you can comment on or make changes to this bug.