1225650 - Use stable hashing for JSObjectToWrappedJSMap

Status: RESOLVED FIXED

(Core :: JavaScript: GC, defect)

Description

[Terrence Cole [:terrence]]

Attachment: stable_JSObject2WrappedJSMap-v0.diff

https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7bdfb39a67c

I'm not yet really happy with this. The way we have to use JS_UpdateWeakPointerAfterGC is really awkward -- it nulls the pointer inline which is not what we want. I think I'd like to expose something like IAtbF to the browser. If I do, I'll make this a two-parter.

Comment 1

[Terrence Cole [:terrence]]

Attachment: export_EdgeNeedsSweep-v0.diff

This doesn't seem too bad.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Terrence Cole [:terrence] from comment #0)

> I'm not yet really happy with this. The way we have to use
> JS_UpdateWeakPointerAfterGC is really awkward -- it nulls the pointer inline
> which is not what we want. I think I'd like to expose something like IAtbF
> to the browser. If I do, I'll make this a two-parter.

We originally had this but it was removed in favour of JS_UpdateWeakPointerAfterGC following review comments for compacting GC.  See bug 650161 comment 96 and onwards.

Comment 3

[Jon Coppeard (:jonco)]

Comment on attachment 8688689 [details] [diff] [review]
stable_JSObject2WrappedJSMap-v0.diff

Review of attachment 8688689 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/xpconnect/src/XPCMaps.cpp
@@ +122,5 @@
>          JS_UpdateWeakPointerAfterGCUnbarriered(&obj);
>          if (!obj)
>              e.removeFront();
> +        else
> +            e.front().mutableKey() = obj;

Can we just do JS_UpdateWeakPointerAfterGCUnbarriered(&e.front().mutableKey()) here?

But yes, it looks like we want IAtbF here.

Comment 4

[Jon Coppeard (:jonco)]

Comment on attachment 8688711 [details] [diff] [review]
export_EdgeNeedsSweep-v0.diff

Review of attachment 8688711 [details] [diff] [review]:
-----------------------------------------------------------------

::: js/public/TracingAPI.h
@@ +345,5 @@
>  
>  typedef js::HashSet<Zone*, js::DefaultHasher<Zone*>, js::SystemAllocPolicy> ZoneSet;
> +
> +// The following functions report whether an edge points to a thing which will
> +// be recycled, and will thus unsafe to point to, at the end of the GC.

Can we make this comment stronger?  Maybe something like:

The following function reports whether a weak reference to a GC thing must be removed.  This happens when the GC finds the target is no longer strongly reachable and schedules it for destruction.

@@ +348,5 @@
> +// The following functions report whether an edge points to a thing which will
> +// be recycled, and will thus unsafe to point to, at the end of the GC.
> +template <typename T>
> +extern JS_PUBLIC_API(bool)
> +EdgeNeedsSweep(T* edge);

Well this is certainly a better name than IsAboutToBeFinalized!

Comment 5

[Terrence Cole [:terrence]]

Actually, it occurred to me that Steve should be landing the GCHashMap::sweep patch any time now, at which point we can move this into a policy and them just call sweep here.

Comment 6

[Terrence Cole [:terrence]]

(In reply to Jon Coppeard (:jonco) (PTO until 4th Jan) from comment #3)
> Comment on attachment 8688689 [details] [diff] [review]
> stable_JSObject2WrappedJSMap-v0.diff
> 
> Review of attachment 8688689 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> ::: js/xpconnect/src/XPCMaps.cpp
> @@ +122,5 @@
> >          JS_UpdateWeakPointerAfterGCUnbarriered(&obj);
> >          if (!obj)
> >              e.removeFront();
> > +        else
> > +            e.front().mutableKey() = obj;
> 
> Can we just do
> JS_UpdateWeakPointerAfterGCUnbarriered(&e.front().mutableKey()) here?

It looks like yes, e.removeFront() does not require an identity check, but I'm really just extremely uncomfortable with nulling the key while the hash is still present.

> But yes, it looks like we want IAtbF here.

I think instead I'll try to use GCHashMap here. I'll land only the first patch and open a new bug for that.

Comment 7

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/3119ae778211ff35d5a8c3d5bdc26589bf8a9452
Bug 1225650 - Use stable hashing for JSObject2WrappedJSMap; r=jonco

Comment 8

[Terrence Cole [:terrence]]

Note: try run was at https://treeherder.mozilla.org/#/jobs?repo=try&revision=a7bdfb39a67c but I ran it before filing the bug so it didn't show up here.

Comment 9

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

https://hg.mozilla.org/mozilla-central/rev/3119ae778211