666549 - Deleting empty weak maps during marking interferes with parallel marking

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Igor Bukanov]

+++ This bug was initially created as a clone of Bug #638660 +++

Currently an empty weak map is deleted from the JS object during the marking phase of the GC. As it mutates the state, this complicates the implementation of parallel marking in bug 638660. To simplify that it would be to shrink the table during sweeping.

Comment 1

[Gregor Wagner [:gwagner]]

Igor do you have any estimation when you can take a look at this? It would be good if we could push the parallel marking patch right after the next aurora merge to get maximum test coverage.

Comment 2

[Andrew McCreight [:mccr8]]

I looked at this a little bit.  It isn't as trivial as I thought it might be because marking has the entire JS Object that contains the map (which is needed to delete and then zero out the map pointer), while in sweeping, we only have the map itself.

Do we need a pointer from the map back to the containing object?  Do we need a separate list of objects that have empty maps that need to be cleared out?  Is it really such a huge deal to free empty maps?

If mutation (aside from mark bits, I assume) is a problem, then how do you deal with the mutation of the gcWeakMapList in the JS runtime?  Is that going to be a problem, too?

(CCing jimb and jorendorff who have been working on WeakMaps recently)

Comment 3

[Jim Blandy :jimb]

Seems like for this you want a hook that's run when the object *survives* GC. I don't think there's any such thing.

Comment 4

[Jim Blandy :jimb]

But... I don't really understand why it's so critical to delete the WeakMap anyway. We could just leave it there, unless we think it's going to be common to have gajillions of empty WeakMaps around.

Comment 5

[Jim Blandy :jimb]

That is, I would r+ a patch that changed the code to simply not delete empty C++ WeakMaps from JS WeakMap objects at all.

Comment 6

[Igor Bukanov]

(In reply to comment #4)
> We could just leave it there, unless we think it's going to be
> common to have gajillions of empty WeakMaps around.

100% agree with that. As HashMap shrinks its capacity during entry removal, we should worry about killing maps only if it hurts.

Comment 7

[Gregor Wagner [:gwagner]]

(In reply to comment #6)
> (In reply to comment #4)
> > We could just leave it there, unless we think it's going to be
> > common to have gajillions of empty WeakMaps around.
> 
> 100% agree with that. As HashMap shrinks its capacity during entry removal,
> we should worry about killing maps only if it hurts.

Great if everybody agrees! Igor do you think you could finish this in the next few days?

Comment 8

[Andrew McCreight [:mccr8]]

244     if (map) {
245         if (IS_GC_MARKING_TRACER(trc) && map->empty()) {
246             trc->context->delete_(map);
247             obj->setPrivate(NULL);
248         } else {
249             map->trace(trc);
250         }
251     }

-->

244     if (map)
249         map->trace(trc);

Comment 9

[Andrew McCreight [:mccr8]]

I can write that up as a real patch tomorrow if nobody else wants to do it first.  I think that's all that's needed.

Comment 10

[Andrew McCreight [:mccr8]]

Attachment: Don't delete empty ObjectValueMaps from WeakMaps

This is just the patch I wrote a few comments above.  Don't delete empty maps.  Deleting empty weak maps seems like a fairly minor optimization, and apparently interferes with parallel marking by making marking manipulate the fields of objects.

I've compiled this patch, but I haven't tested it yet.

Comment 11

[Andrew McCreight [:mccr8]]

Comment on attachment 546545 [details] [diff] [review]
Don't delete empty ObjectValueMaps from WeakMaps

I was able to do basic browsing just fine.

I just pushed this to try: http://tbpl.mozilla.org/?tree=Try&rev=48413cf1f373

Andreas, do you agree that it is okay to do this deoptimization?

Gregor or Igor, is this change sufficient to fix the parallel marking problem?

Comment 12

[Andreas Gal :gal]

Sure, I don't think this hurts much in practice.

Comment 13

[Gregor Wagner [:gwagner]]

(In reply to comment #11)
> 
> Gregor or Igor, is this change sufficient to fix the parallel marking
> problem?

It fixes the problem in WeakMap_mark but we still need to lock in
 WeakMapBase::trace because of:
            JSRuntime *rt = tracer->context->runtime;
            next = rt->gcWeakMapList;
            rt->gcWeakMapList = this;

Comment 14

[Andrew McCreight [:mccr8]]

At least conceptually, I'd think you could have each individual marking thread gather its own list of weak maps, and combine them together at the end.  It is just a linked list of WeakMaps the marking has encountered.

Comment 15

[Igor Bukanov]

Comment on attachment 546545 [details] [diff] [review]
Don't delete empty ObjectValueMaps from WeakMaps

> static void
> WeakMap_mark(JSTracer *trc, JSObject *obj)
> {
>     ObjectValueMap *map = GetObjectMap(obj);
>+    if (map)
>+        map->trace(trc);
> }

Micro-nit: use
 
    if (ObjectValueMap *map = GetObjectMap(obj))
        map->trace(trc);

Comment 16

[Andrew McCreight [:mccr8]]

Attachment: Don't delete empty ObjectValueMaps from WeakMaps

Addressed review comment.  Carrying forward igor's review.

Comment 17

[Andrew McCreight [:mccr8]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/b8785cf10611

Comment 18

[Marco Bonardo [:mak]]

http://hg.mozilla.org/mozilla-central/rev/b8785cf10611