Closed Bug 1081379 Opened 10 years ago Closed 10 years ago

land Nightly-only ArrayBuffer.transfer

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla36

People

(Reporter: luke, Assigned: luke)

References

Details

(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])

Attachments

(5 files, 1 obsolete file)

Attached patch add-ab-transfer (obsolete) — Splinter Review
ArrayBuffer.transfer was discussed at the last TC39 meeting and advanced to stage 0 (from stage -1 I guess?).  Here is the strawman:
  https://gist.github.com/andhow/95fb9e49996615764eff

Brendan and Allen thought it'd be useful to land an experimental (#ifdef NIGHTLY) version so that's what this patch is (next next one will add specific optimizations for asm.js-ified ArrayBuffers).

Feedback welcome on the design (or impl).  If the proposal advances to a later stage (3), we can consider shipping this feature on release.
Attachment #8503468 - Flags: review?(sphink)
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Comment on attachment 8503468 [details] [diff] [review]
add-ab-transfer

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +323,5 @@
> +            return false;
> +        }
> +
> +        if (oldBuffer->zone() != cx->zone())
> +            cx->updateMallocCounter(oldByteLength);

I don't understand why this is oldByteLength instead of newByteLength, and I don't think this should be updating the malloc counter at all -- JS_NewArrayBufferWithContents will update the counter when it takes ownership of the pointer in ArrayBufferObject::create.

That does mean that we'll double-count buffer data when you transfer within a zone. But right now, it's really an allocation counter, not a memory size counter. We want to move it to a memory size counter, which means passing the size to js_free everywhere, but for now I think you'll just have to let it double-count.

(Transferring buffers between zones will double-count too, since the old zone's counter won't be decremented.)
Attachment #8503468 - Flags: review?(sphink) → review+
Ah, I didn't notice that ARrayBufferObject::create would increment the counter.  My goal was just to make sure that the target compartment was docked.  I'll remove both the updateMallocCounter calls, then.

Btw, the reason for oldByteLength in the first updateMallocCounter was because, in case of growth, (newByteLength-oldByteLength) would be added as well, so the sum was newByteLength.
Also, while I was here, I noticed js_InitArrayBufferClass is in the wrong file, so this patch moves it.
Attachment #8504232 - Flags: review?(sphink)
(In reply to Luke Wagner [:luke] from comment #2)
> Ah, I didn't notice that ARrayBufferObject::create would increment the
> counter.  My goal was just to make sure that the target compartment was
> docked.  I'll remove both the updateMallocCounter calls, then.
> 
> Btw, the reason for oldByteLength in the first updateMallocCounter was
> because, in case of growth, (newByteLength-oldByteLength) would be added as
> well, so the sum was newByteLength.

Wow. I am blind. Somehow, I just didn't see the 2nd updateMallocCounter call.
Comment on attachment 8504232 [details] [diff] [review]
move-init-array-buffer

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

I vaguely remember that there was originally some (weird, unfortunate) reason for where it was. But if it compiles now, then yay!
Attachment #8504232 - Flags: review?(sphink) → review+
In preparing the final patch (which optimizes the asm.js-mapped-buffer case), I realized that it's not really a good idea to treat "mapped" and "asm.js" as orthogonal flags, since the meaning of "mapped" is very different whether or not the "asm.js" flag is present.

This patch tweaks BufferKind so that it is a simple enum, not a bitfield and splits out "asm.js" into two cases (malloced and mapped).  With a simple enum, we can now switch over all the cases which simplifies a fair amount of code.
Attachment #8503468 - Attachment is obsolete: true
Attachment #8505207 - Flags: review?(sphink)
Based on the previous patch, this patch changes the hasStealableContents() predicate so it doesn't rule out all asm.js buffers and adds hasMallocedContents() which is what most of the callers really mean to require.
Attachment #8505209 - Flags: review?(sphink)
Attached patch add-ab-transferSplinter Review
This is the original ArrayBuffer.transfer patch, but rebased on top of the previous two.  The newByteLength = 0 case is now handled by calling neuter() instead of stealContents followed by js_free (which was needlessly inefficient in some cases).
Attachment #8505210 - Flags: review?(sphink)
Here's the final patch that optimizes the isAsmJSMapped case by just protecting/unprotecting the pages of the 4gb region in-place.
Attachment #8505211 - Flags: review?(sphink)
Just the mv js_InitArrayBufferClass patch:
https://hg.mozilla.org/integration/mozilla-inbound/rev/397f83283b72
Keywords: leave-open
Comment on attachment 8505207 [details] [diff] [review]
change-kind-flags

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

::: js/src/vm/ArrayBufferObject.h
@@ +313,5 @@
>      void setOwnsData(OwnsState owns) {
>          setFlags(owns ? (flags() | OWNS_DATA) : (flags() & ~OWNS_DATA));
>      }
>  
> +    void setIsAsmJSMalloced() { setFlags(flags() | ASMJS_MALLOCED); }

This assumes something about the previous state. How about setFlags((flags() & ~KIND_MASK) | ASMJS_MALLOCED)?
Attachment #8505207 - Flags: review?(sphink) → review+
Attachment #8505209 - Flags: review?(sphink) → review+
Attachment #8505210 - Flags: review?(sphink) → review+
Comment on attachment 8505211 [details] [diff] [review]
optimize-ab-transfer

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

::: js/src/vm/ArrayBufferObject.cpp
@@ +282,5 @@
> +        ArrayBufferObject::stealContents(cx, oldBuffer, /* hasStealableContents = */ true);
> +    if (!stolen)
> +        return false;
> +
> +    uint8_t *data = stolen.data();

assert stolen.kind == ASMJS_MAPPED or whatever it's called?

@@ +403,5 @@
>      } else {
> +# ifdef JS_CPU_X64
> +        // With a 4gb mapped asm.js buffer, we can simply enable/disable access
> +        // to the delta as long as the requested length is page-sized.
> +        if (oldBuffer->isAsmJSMapped() && newByteLength % AsmJSPageSize == 0)

For people like me who are a little weak on precedence rules, can you parenthesize (newByteLength % AsmJSPageSize)?
Attachment #8505211 - Flags: review?(sphink) → review+
(In reply to Steve Fink [:sfink] from comment #12)
> This assumes something about the previous state. How about setFlags((flags()
> & ~KIND_MASK) | ASMJS_MALLOCED)?

Wow, I totally skipped over this (and just got lucky that it worked); good catch.
Keywords: leave-open
Blocks: 1084985
The Gist in comment 0 is dead. Here it is again: https://gist.github.com/lukewagner/2735af7eea411e18cf20

Documented:
https://developer.mozilla.org/en-US/docs/Web/JavaScript/Reference/Global_Objects/ArrayBuffer/transfer
Not an Fx 36 for developers, because it is Nightly-only and will not ride the trains.
Depends on: 1150858
You need to log in before you can comment on or make changes to this bug.