The internal layout of ArrayBufferObjects is an enormous mess. Most of this stems from the fact that ArrayBufferObjects use their |elements| for data storage, which I think goes back to the desire to use inline space in the object for storing data when possible. So I think this was originally partly my fault but it has since metastasized and there are all sorts of horrible workarounds in place to allow this behavior to continue --- complicated public interfaces exposing both the data and elements header pointers, abusing the fields of ObjectElements to store array buffer state like flags and view lists, using delegate objects to hold user properties, embedding state about the array buffer itself in its views, array buffer logic bleeding into the rest of the VM, and so forth. This situation should be fixed just for sanity's sake, but it also makes it really hard to do new things with ArrayBuffers, like allowing them to point to data they don't own or to create them lazily. The attached patch making the following changes: - Makes array buffers normal native objects, with reserved slots for their data pointer, byte length, flags, and view list. - The array buffer's elements act like other native objects. The data pointer can point to either malloc'ed, mmap'ed (asm.js) or inline storage and doesn't have any ObjectElements header or anything. - Simplify public interfaces to only include data pointers and not object elements headers. - Removes the BUFFER_LINK list threaded through the first view of each array buffer, instead maintaining a vector of live array buffers encountered during GC on the compartment. This removes a lot of pointer chasing and saves memory in buffer views. The only real drawback to this change is that ArrayBufferObjects can no longer be nursery allocated, since they have a finalizer (which, however, can be called on the background thread so the objects are still background finalizable). This needs a more general purpose fix I think so that objects with finalizers can in some cases be in the nursery, e.g. we don't keep track of them if they don't own any data outside the nursery (the buffer's data is inline or is itself in the nursery) and put the object in a table like |hugeSlots| if that changes. But that's outside the scope of this bug and doing it right now could interfere with GGC landing I think.

Actually, we never allocate ABOs with unowned or asm.js buffers in the nursery, so we should be able to just continue using the existing hugeSlots mechanism. I do concur, however, that this is not critical for our initial GGC landing and we should do it as a follow-up. I'll file a bug to implement that, so r=me on the GC changes in this patch.

Comment on attachment 8385513 [details] [diff] [review] patch (cbe54c837281) Review of attachment 8385513 [details] [diff] [review]: ----------------------------------------------------------------- It's way better without all this cruft. (Most of which I added.) I'm a little nervous about the perf effects, but this + perf fixes are a far better place to be than the current state. ::: js/src/builtin/TypedObjectConstants.h @@ +117,2 @@ > > #define JS_TYPEDOBJ_SLOT_DATA 7 // private slot, based on alloc kind Looks like JS_TYPEDOBJ_SLOT_DATA should be dropped to 6 too. ::: js/src/jsapi.h @@ +3141,5 @@ > > /* > * Steal the contents of the given array buffer. The array buffer has its > * length set to 0 and its contents array cleared. The caller takes ownership > * of |*contents| and must free it or transfer ownership via takes ownership of the return value now, not *contents. @@ +3156,3 @@ > */ > +extern JS_PUBLIC_API(void *) > +JS_AllocateArrayBufferContents(JSContext *maybecx, uint32_t nbytes); Is this API entry still needed? Isn't it just JS_malloc now?

Comment on attachment 8385513 [details] [diff] [review] patch (cbe54c837281) Review of attachment 8385513 [details] [diff] [review]: ----------------------------------------------------------------- This looks great! I think sfink is the appropriate reviewer and it seems like he's already started. One request: can you revert all the removals in testNeuter.js? I don't know if that was intentional, but we still want those tests...

This change could be useful for bug 855669 where it is best to align the buffer data to a page boundary. It makes it more efficient to allocate pages for the buffer elements - it had been necessary to allocate a preceding page to hold the header but this should no longer be necessary. This change might also help optimize asm.js heap access by allowing guard pages to be placed before the data. The compiler could then optimize b[(p & m) - c] to avoid a bounds check for a small range of constant offsets 'c', and it might be possible to hoist the mask.

Comment on attachment 8385513 [details] [diff] [review] patch (cbe54c837281) Review of attachment 8385513 [details] [diff] [review]: ----------------------------------------------------------------- Ugh, this'll probably duplicate my earlier review comments. It's way better without all this cruft. (Most of which I added.) I'm a little nervous about the perf effects, but this + perf fixes are a far better place to be than the current state. I'm worried about the perf effects of this, though, especially pre-GGC. I remember that preventing typed arrays from being background-finalizable had a huge effect on some game -- the BananaBread benchmark, maybe? Can't remember. The b2g guys really want to get bug 945152 in before the Mar 17 split. It's going to interfere with this patch. It might be safest to delay landing this until the split. ::: js/src/jit-test/tests/asm.js/testNeuter.js @@ -14,5 @@ > } > return {get:get, set:set} > } > -if (isAsmJSCompilationAvailable()) > - assertEq(isAsmJSModule(f), true); Yeah, not sure why you're removing this test. ::: js/src/jsapi.h @@ +3141,5 @@ > > /* > * Steal the contents of the given array buffer. The array buffer has its > * length set to 0 and its contents array cleared. The caller takes ownership > * of |*contents| and must free it or transfer ownership via takes ownership of the return value now, not *contents. @@ +3156,3 @@ > */ > +extern JS_PUBLIC_API(void *) > +JS_AllocateArrayBufferContents(JSContext *maybecx, uint32_t nbytes); Is this API entry still needed? Isn't it just JS_malloc now? @@ +3164,3 @@ > */ > +extern JS_PUBLIC_API(void *) > +JS_ReallocateArrayBufferContents(JSContext *cx, uint32_t nbytes, void *oldContents, uint32_t oldNbytes); Seems like this API could be replaced with something that just replaces the data pointer with another, but never mind for now.

(In reply to Steve Fink [:sfink] from comment #5) > Comment on attachment 8385513 [details] [diff] [review] > patch (cbe54c837281) > > Review of attachment 8385513 [details] [diff] [review]: > ----------------------------------------------------------------- ... > @@ +3156,3 @@ > > */ > > +extern JS_PUBLIC_API(void *) > > +JS_AllocateArrayBufferContents(JSContext *maybecx, uint32_t nbytes); > > Is this API entry still needed? Isn't it just JS_malloc now? fwiw For the benefit of the buffer.discard() work the data will want to be page aligned, and it might be best to just mmap a region if it is page aligned. There might be some logic to decide if it will be malloced or mmapped and perhaps keeping JS_AllocateArrayBufferContents to contain this logic would be appropriate.

Your timing was impeccable. :-( This change substantially interfered with fixing a security bug, and given the time frame for doing that, sfink and I agreed the right tradeoff was to back this out for now. (Along with two other patches to the same code. :-\ ) Once the dust's settled on those changes, and on relanding the two other patches, this can go back in. https://hg.mozilla.org/integration/mozilla-inbound/rev/ad76a457e582

https://hg.mozilla.org/mozilla-central/rev/ce6a8fa5db7d https://hg.mozilla.org/mozilla-central/rev/ff97f449b232