1475226 - Use JSContext for malloc allocation rather than Zone where possible

Status: RESOLVED FIXED

(Core :: JavaScript: GC, enhancement)

Description

[Jon Coppeard (:jonco)]

Attachment: allocate-on-context

Both JSContext and Zone are MallocProviders and provide various pod_malloc style methods.  Currently we use the Zone ones in most places, but this has a few drawbacks:

 - it's different to GC thing allocation where we pass the context to indicate the zone
 - it doesn't report failure to the context which we want in most places
 - we have to call zone() on the context to get the Zone anyway

This patch makes most allocations use the context instead, which the exception of places where there isn't a current context (e.g. the GC) and long lived data structures associated with a Zone (e.g. a JS Map object).

Comment 1

[Steve Fink [:sfink] [:s:]]

Comment on attachment 8991610 [details] [diff] [review]
allocate-on-context

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

I can't argue with the number of lines removed by this patch!

The only thing that makes me nervous is whether the cx is in the right zone, but it looks like you've covered that in various places with asserts or AutoRealm.

::: js/src/gc/Allocator.cpp
@@ +110,5 @@
>                                 size_t nDynamicSlots)
>  {
>      HeapSlot* slots = nullptr;
>      if (nDynamicSlots) {
> +        slots = cx->maybe_pod_malloc<HeapSlot>(nDynamicSlots);

This one doesn't seem right. If CanGC, it would report OOM twice.

If NoGC, then it will now report OOM -- but ReportOutOfMemory(cx) cannot GC, so I'm not sure why that's a problem?

::: js/src/gc/Nursery-inl.h
@@ +114,5 @@
>  static inline T*
>  AllocateObjectBuffer(JSContext* cx, JSObject* obj, uint32_t count)
>  {
>      if (cx->helperThread())
> +        return cx->pod_malloc<T>(count);

Hm, it looks like this is a bugfix? It doesn't seem like it would have reported OOM before, but should.

Comment 2

[Jon Coppeard (:jonco)]

(In reply to Steve Fink [:sfink] [:s:] from comment #1)

> The only thing that makes me nervous is whether the cx is in the right zone,
> but it looks like you've covered that in various places with asserts

Oh, I actually meant to make those assertSameCompartment.  I'll update that.

> > +        slots = cx->maybe_pod_malloc<HeapSlot>(nDynamicSlots);
> 
> This one doesn't seem right. If CanGC, it would report OOM twice.

The 'maybe' versions don't report on failure, so this should have the same behaviour as before.

> Hm, it looks like this is a bugfix? It doesn't seem like it would have
> reported OOM before, but should.

Yes, this is a fix.

Comment 3

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/integration/mozilla-inbound/rev/76de95da94dd
Use JSContext for malloc allocation rather than Zone where possible r=sfink

Comment 4

[Noemi Erli[:noemi_erli]]

https://hg.mozilla.org/mozilla-central/rev/76de95da94dd