Closed
Bug 1081379
Opened 10 years ago
Closed 10 years ago
land Nightly-only ArrayBuffer.transfer
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: luke, Assigned: luke)
References
Details
(Keywords: dev-doc-complete, Whiteboard: [DocArea=JS])
Attachments
(5 files, 1 obsolete file)
4.67 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
23.09 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
4.63 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
9.03 KB,
patch
|
sfink
:
review+
|
Details | Diff | Splinter Review |
9.20 KB,
patch
|
sfink
:
review+
|
Details | Diff | 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)
Updated•10 years ago
|
Keywords: dev-doc-needed
Whiteboard: [DocArea=JS]
Comment 1•10 years ago
|
||
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+
Assignee | ||
Comment 2•10 years ago
|
||
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.
Assignee | ||
Comment 3•10 years ago
|
||
Also, while I was here, I noticed js_InitArrayBufferClass is in the wrong file, so this patch moves it.
Attachment #8504232 -
Flags: review?(sphink)
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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)
Assignee | ||
Comment 7•10 years ago
|
||
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)
Assignee | ||
Comment 8•10 years ago
|
||
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)
Assignee | ||
Comment 9•10 years ago
|
||
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)
Assignee | ||
Comment 10•10 years ago
|
||
Just the mv js_InitArrayBufferClass patch: https://hg.mozilla.org/integration/mozilla-inbound/rev/397f83283b72
Keywords: leave-open
Comment 12•10 years ago
|
||
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+
Updated•10 years ago
|
Attachment #8505209 -
Flags: review?(sphink) → review+
Updated•10 years ago
|
Attachment #8505210 -
Flags: review?(sphink) → review+
Comment 13•10 years ago
|
||
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+
Assignee | ||
Comment 14•10 years ago
|
||
(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.
Assignee | ||
Comment 15•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/6e59ea43d686 https://hg.mozilla.org/integration/mozilla-inbound/rev/1d6e0f775724 https://hg.mozilla.org/integration/mozilla-inbound/rev/ec90e1a7b632 https://hg.mozilla.org/integration/mozilla-inbound/rev/c837c78af266
Assignee | ||
Updated•10 years ago
|
Keywords: leave-open
Comment 16•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/c837c78af266 https://hg.mozilla.org/mozilla-central/rev/ec90e1a7b632 https://hg.mozilla.org/mozilla-central/rev/1d6e0f775724 https://hg.mozilla.org/mozilla-central/rev/6e59ea43d686
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
Comment 17•9 years ago
|
||
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.
Keywords: dev-doc-needed → dev-doc-complete
You need to log in
before you can comment on or make changes to this bug.
Description
•