Closed
Bug 1081277
Opened 10 years ago
Closed 10 years ago
OdinMonkey: loosen detachment limitations
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
mozilla36
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(3 files, 3 obsolete files)
6.86 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
18.39 KB,
patch
|
bbouvier
:
review+
sfink
:
review+
|
Details | Diff | Splinter Review |
27.19 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
To land an experimental implementation of ArrayBuffer.transfer (#ifdef NIGHTLY for now), we need to support detaching an ArrayBuffer linked to an asm.js module while that asm.js module is on the stack (since this is the primary use case via malloc).
Assignee | ||
Comment 1•10 years ago
|
||
This small patch maintains a list of live AsmJSModules so that they can be traversed in a later patch.
Attachment #8503359 -
Flags: review?(benj)
Assignee | ||
Comment 2•10 years ago
|
||
This patch moves the current "throw if trying to detach while asm.js is running" logic into js/src/asmjs and changes it to use the list-of-modules in the last patch. What happens in js/src/asmjs is about to change in the next patch.
Attachment #8503371 -
Flags: review?(sphink)
Attachment #8503371 -
Flags: review?(benj)
Assignee | ||
Comment 3•10 years ago
|
||
Oops, this is the patch.
Attachment #8503371 -
Attachment is obsolete: true
Attachment #8503371 -
Flags: review?(sphink)
Attachment #8503371 -
Flags: review?(benj)
Attachment #8503374 -
Flags: review?(sphink)
Attachment #8503374 -
Flags: review?(benj)
Assignee | ||
Comment 4•10 years ago
|
||
This patch delays throwing until returning from the FFI. This gives the FFI the change to switch heaps (via change-heap) and avoid an error at all (which is what will happen in the case of ArrayBuffer.transfer: transfer will detach the original heap, hand the contenst to a new heap, and change-heap to that). This still isn't exactly correct (the exception should be delayed until an actual heap access is attempted, but it's strictly better than the current state and a ton less work, so I'd like to punt on doing the fully correct thing a bit longer).
Attachment #8503377 -
Flags: review?(benj)
Comment 5•10 years ago
|
||
Comment on attachment 8503359 [details] [diff] [review] maintain-list Review of attachment 8503359 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/Runtime.h @@ +1083,5 @@ > /* AsmJSCache callbacks are runtime-wide. */ > + JS::AsmJSCacheOps asmJSCacheOps; > + > + /* Head of the linked list of linked asm.js modules. */ > + js::AsmJSModule *linkedAsmJSModules; nit: could you please initialize it in Runtime.cpp, to avoid ASAN's fury and random behavior?
Attachment #8503359 -
Flags: review?(benj) → review+
Assignee | ||
Comment 6•10 years ago
|
||
Rebase with some tweaks (JSMSG_OUT_OF_MEMORY) and better comments.
Attachment #8503374 -
Attachment is obsolete: true
Attachment #8503374 -
Flags: review?(sphink)
Attachment #8503374 -
Flags: review?(benj)
Attachment #8504227 -
Flags: review?(sphink)
Attachment #8504227 -
Flags: review?(benj)
Assignee | ||
Comment 7•10 years ago
|
||
Rebase, better comments, and JSMSG_OUT_OF_MEMORY.
Attachment #8503377 -
Attachment is obsolete: true
Attachment #8503377 -
Flags: review?(benj)
Attachment #8504229 -
Flags: review?(benj)
Comment 8•10 years ago
|
||
Comment on attachment 8504227 [details] [diff] [review] handle-detach-in-asmjs Review of attachment 8504227 [details] [diff] [review]: ----------------------------------------------------------------- Looks good. ::: js/src/asmjs/AsmJSModule.cpp @@ +868,5 @@ > > restoreHeapToInitialState(maybePrevBuffer); > } > > +bool (this could be void, but i guess the next patch makes it actually fallible...)
Attachment #8504227 -
Flags: review?(benj) → review+
Comment 9•10 years ago
|
||
Comment on attachment 8504229 [details] [diff] [review] delay-neuter-error Review of attachment 8504229 [details] [diff] [review]: ----------------------------------------------------------------- Simple and clear. r+ if you delete the changes made in AsmJSFrameIterator*, otherwise there seems to be missing something in the detachHeap exit (or there is some magic happening which i am not seeing). ::: js/src/asmjs/AsmJSFrameIterator.h @@ +71,5 @@ > Reason_None, > Reason_IonFFI, > Reason_SlowFFI, > Reason_Interrupt, > + Reason_OnDetached, Unless I am missing something, this is unused? ::: js/src/asmjs/AsmJSModule.cpp @@ +908,5 @@ > js::OnDetachAsmJSArrayBuffer(JSContext *cx, Handle<ArrayBufferObject*> buffer) > { > for (AsmJSModule *m = cx->runtime()->linkedAsmJSModules; m; m = m->nextLinked()) { > + if (m->maybeHeapBufferObject() && !m->detachHeap(cx)) > + return false; This will silently fail. Could we have an error message at this point? The OOM message isn't the right one anymore, though. ::: js/src/asmjs/AsmJSValidate.cpp @@ +7641,5 @@ > +// call change-heap to another heap (viz., the new heap returned by transfer) > +// before returning to asm.js code. If the application fails to do this (if the > +// heap pointer is null), jump to a stub to handle this. > +static void > +CheckForHeapDetachment(ModuleCompiler &m, Register scratch) The Check* prefix in this file often refers to type checking, could this be GenerateCheckForHeapDetachment, or something that makes it clearer that it generates code? That would be symmetric with GenerateOnDetachedLabelExit @@ +7651,5 @@ > + AssertStackAlignment(masm, ABIStackAlignment); > +#if defined(JS_CODEGEN_X86) > + CodeOffsetLabel label = masm.movlWithPatch(PatchedAbsoluteAddress(), scratch); > + masm.append(AsmJSGlobalAccess(label, AsmJSHeapGlobalDataOffset)); > + masm.branchTestPtr(Assembler::Equal, scratch, scratch, &m.onDetachedLabel()); here and below, please use Assembler::Zero instead of Assembler::Equal. They're the same on x86 and x64 at least, but testing zero seems to make more sense in this case. ::: js/src/jit-test/tests/asm.js/testNeuter.js @@ +75,5 @@ > +new Int32Array(buf3)[13] = 1024; > +new Int32Array(buf4)[13] = 1337; > + > +function ffi2() { neuter(buf1, "change-data"); assertEq(changeHeap(buf2), true); } > +var {changeHeap, get:get2, inner} = asmLink(m, this, {ffi:ffi2}, buf1); destructured assignments with renaming? w00t ::: js/src/jit-test/tests/asm.js/testProfiling.js @@ +129,5 @@ > +var buf = new ArrayBuffer(BUF_CHANGE_MIN); > +var ffi = function() { neuter(buf, 'change-data') } > +var f = asmLink(asmCompile('g','ffis','buf', USE_ASM + 'var ffi = ffis.ffi; var i32 = new g.Int32Array(buf); function f() { ffi() } return f'), this, {ffi:ffi}, buf); > +enableSingleStepProfiling(); > +assertThrowsInstanceOf(()=>f(), TypeError); it seems it could just be assertThrowsInstanceOf(f, TypeError);
Attachment #8504229 -
Flags: review?(benj) → review+
Assignee | ||
Comment 10•10 years ago
|
||
(In reply to Benjamin Bouvier [:bbouvier] from comment #9) All great points, thanks.
Comment 11•10 years ago
|
||
Comment on attachment 8504227 [details] [diff] [review] handle-detach-in-asmjs Review of attachment 8504227 [details] [diff] [review]: ----------------------------------------------------------------- ::: js/src/vm/ArrayBufferObject.cpp @@ +697,5 @@ > buffer->setOwnsData(DoesntOwnData); > + if (!ArrayBufferObject::neuter(cx, buffer, newContents)) { > + js_free(newContents.data()); > + return BufferContents::createUnowned(nullptr); > + } I think we can finally get rid of the uncommitted backing buffer now, but I guess it should wait for the current round of fixes first.
Attachment #8504227 -
Flags: review?(sphink) → review+
Assignee | ||
Comment 12•10 years ago
|
||
https://hg.mozilla.org/integration/mozilla-inbound/rev/88497210b950 https://hg.mozilla.org/integration/mozilla-inbound/rev/20aa7722f330 https://hg.mozilla.org/integration/mozilla-inbound/rev/62185aa0fd86
Comment 13•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/88497210b950 https://hg.mozilla.org/mozilla-central/rev/20aa7722f330 https://hg.mozilla.org/mozilla-central/rev/62185aa0fd86
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla36
You need to log in
before you can comment on or make changes to this bug.
Description
•