Closed Bug 1235631 Opened 9 years ago Closed 9 years ago

Odin: remove change-heap support

Categories

(Core :: JavaScript Engine, defect)

defect
Not set
normal

Tracking

()

RESOLVED FIXED
mozilla46
Tracking Status
firefox46 --- fixed

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(1 file, 2 obsolete files)

Attached patch rm-change-heap (obsolete) — Splinter Review
Attachment #8702672 - Flags: review?(benj)
Attached patch rm-change-heap (obsolete) — Splinter Review
Fixup test outside tests/asm.js.
Attachment #8702672 - Attachment is obsolete: true
Attachment #8702672 - Flags: review?(benj)
Attachment #8702675 - Flags: review?(benj)
Attached patch rm-change-heapSplinter Review
This version also removes ArrayBuffer.transfer: if it's not removed, then there is a significant risk that lingering code compiled with "-s ALLOW_MEMORY_GROWTH=1" will use ArrayBuffer.transfer (only on Nightly) which, with neutering-support removed, would throw. There doesn't appear to be any movement on transfer in TC39 and no intention to implement in other engines, so this seems like the right thing to do at this time. It's easy enough to resurrect later.
Attachment #8702675 - Attachment is obsolete: true
Attachment #8702675 - Flags: review?(benj)
Attachment #8702681 - Flags: review?(benj)
Comment on attachment 8702681 [details] [diff] [review] rm-change-heap Review of attachment 8702681 [details] [diff] [review]: ----------------------------------------------------------------- BURN IT WITH FIRE ::: js/src/asmjs/WasmModule.cpp @@ +515,2 @@ > MOZ_ASSERT_IF(heap->is<ArrayBufferObject>(), heap->as<ArrayBufferObject>().isAsmJS()); > + MOZ_ASSERT(!heap_); Wow, the proximity of this assertion and the hasHeap() above seems misleading here. How about keeping the name usesHeap() instead of hasHeap()? or renaming it needsHeap()? ::: js/src/asmjs/WasmModule.h @@ +327,5 @@ > > +// A wasm module can either use no heap, a unshared heap (ArrayBuffer) or shared > +// heap (SharedArrayBuffer). > + > +enum class HeapUse I'd say HeapUsage, but that's probably me not knowing the differences between Use and Usage in english, so trusting your judgement here. ::: js/src/asmjs/WasmStubs.cpp @@ +98,5 @@ > // The signature of the entry point is Module::CodePtr. The exported wasm > // function has an ABI derived from its specific signature, so this function > // must map from the ABI of CodePtr to the export's signature's ABI. > static bool > +GenerateEntry(ModuleGenerator& mg, unsigned exportIndex, HeapUse heapUse) I think you just want a bool hasUse here, not the entire HeapUse? @@ +335,5 @@ > // Generate a stub that is called via the internal ABI derived from the > // signature of the import and calls into an appropriate InvokeImport C++ > // function, having boxed all the ABI arguments into a homogeneous Value array. > static bool > +GenerateInterpExitStub(ModuleGenerator& mg, unsigned importIndex, HeapUse heapUse, heapUse is unused here @@ +440,5 @@ > // Generate a stub that is called via the internal ABI derived from the > // signature of the import and calls into a compatible JIT function, > // having boxed all the ABI arguments into the JIT stack frame layout. > static bool > +GenerateJitExitStub(ModuleGenerator& mg, unsigned importIndex, HeapUse heapUse, You could also just use a boolean hasHeap here. @@ +1072,5 @@ > return mg.defineInlineStub(offsets); > } > > bool > +wasm::GenerateStubs(ModuleGenerator& mg, HeapUse heapUse) and by transitivity, this could use a boolean only as well :)
Attachment #8702681 - Flags: review?(bbouvier+bugzilla) → review+
Agreed on all points, thanks!
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: