The default bug view has changed. See this FAQ.

Deleting empty weak maps during marking interferes with parallel marking

RESOLVED FIXED in mozilla8

Status

()

Core
JavaScript Engine
RESOLVED FIXED
6 years ago
6 years ago

People

(Reporter: Igor Bukanov, Assigned: mccr8)

Tracking

(Blocks: 1 bug)

unspecified
mozilla8
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Reporter)

Description

6 years ago
+++ 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.
Blocks: 638660
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.
(Assignee)

Comment 2

6 years ago
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

6 years ago
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

6 years ago
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

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

Comment 6

6 years ago
(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.
(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?
(Assignee)

Comment 8

6 years ago
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);
(Assignee)

Comment 9

6 years ago
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.
(Assignee)

Comment 10

6 years ago
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.
(Assignee)

Comment 11

6 years ago
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?
Attachment #546545 - Flags: review?(igor)
Attachment #546545 - Flags: feedback?(gal)

Comment 12

6 years ago
Sure, I don't think this hurts much in practice.
(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;
(Assignee)

Comment 14

6 years ago
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.
(Reporter)

Comment 15

6 years ago
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);
Attachment #546545 - Flags: review?(igor) → review+
(Assignee)

Updated

6 years ago
Summary: Delete weak map when the last key is removed or during sweeping, not during the marking → Deleting empty weak maps during marking interferes with parallel marking
(Assignee)

Comment 16

6 years ago
Created attachment 546811 [details] [diff] [review]
Don't delete empty ObjectValueMaps from WeakMaps

Addressed review comment.  Carrying forward igor's review.
Assignee: igor → continuation
Attachment #546545 - Attachment is obsolete: true
Attachment #546811 - Flags: review+
Attachment #546545 - Flags: feedback?(gal)
(Assignee)

Comment 17

6 years ago
http://hg.mozilla.org/integration/mozilla-inbound/rev/b8785cf10611
http://hg.mozilla.org/mozilla-central/rev/b8785cf10611
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla8
You need to log in before you can comment on or make changes to this bug.