Last Comment Bug 666549 - Deleting empty weak maps during marking interferes with parallel marking
: Deleting empty weak maps during marking interferes with parallel marking
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla8
Assigned To: Andrew McCreight [:mccr8]
:
Mentors:
Depends on:
Blocks: 638660
  Show dependency treegraph
 
Reported: 2011-06-23 05:07 PDT by Igor Bukanov
Modified: 2011-07-20 06:51 PDT (History)
21 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Don't delete empty ObjectValueMaps from WeakMaps (990 bytes, patch)
2011-07-18 08:49 PDT, Andrew McCreight [:mccr8]
igor: review+
Details | Diff | Splinter Review
Don't delete empty ObjectValueMaps from WeakMaps (1.03 KB, patch)
2011-07-19 09:46 PDT, Andrew McCreight [:mccr8]
continuation: review+
Details | Diff | Splinter Review

Description Igor Bukanov 2011-06-23 05:07:15 PDT
+++ 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] 2011-06-30 10:20:21 PDT
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] 2011-06-30 11:38:01 PDT
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 2011-06-30 12:20:54 PDT
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 2011-06-30 12:21:36 PDT
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 2011-07-01 10:40:54 PDT
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 2011-07-13 13:08:29 PDT
(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] 2011-07-14 20:37:29 PDT
(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] 2011-07-14 20:41:28 PDT
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] 2011-07-14 20:44:20 PDT
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] 2011-07-18 08:49:39 PDT
Created attachment 546545 [details] [diff] [review]
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] 2011-07-18 10:44:32 PDT
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 2011-07-18 15:44:29 PDT
Sure, I don't think this hurts much in practice.
Comment 13 Gregor Wagner [:gwagner] 2011-07-18 17:56:02 PDT
(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] 2011-07-18 17:59:15 PDT
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 2011-07-19 01:57:54 PDT
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] 2011-07-19 09:46:27 PDT
Created attachment 546811 [details] [diff] [review]
Don't delete empty ObjectValueMaps from WeakMaps

Addressed review comment.  Carrying forward igor's review.
Comment 17 Andrew McCreight [:mccr8] 2011-07-19 09:52:51 PDT
http://hg.mozilla.org/integration/mozilla-inbound/rev/b8785cf10611
Comment 18 Marco Bonardo [::mak] 2011-07-20 06:51:03 PDT
http://hg.mozilla.org/mozilla-central/rev/b8785cf10611

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