Closed Bug 1225650 Opened 6 years ago Closed 6 years ago

Use stable hashing for JSObjectToWrappedJSMap

Categories

(Core :: JavaScript: GC, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox45 --- affected
firefox46 --- fixed

People

(Reporter: terrence, Assigned: terrence)

References

Details

Attachments

(2 files)

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.
Attachment #8688689 - Flags: review?(jcoppeard)
Assignee: nobody → terrence
Status: NEW → ASSIGNED
This doesn't seem too bad.
Attachment #8688711 - Flags: review?(jcoppeard)
(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 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.
Attachment #8688689 - Flags: review?(jcoppeard) → review+
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!
Attachment #8688711 - Flags: review?(jcoppeard) → review+
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.
(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.
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.
https://hg.mozilla.org/mozilla-central/rev/3119ae778211
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.