+++ 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.

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.

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)

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.

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.

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

(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?

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);

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.

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 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?

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;

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 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);

Addressed review comment. Carrying forward igor's review.