Closed
Bug 1235631
Opened 9 years ago
Closed 9 years ago
Odin: remove change-heap support
Categories
(Core :: JavaScript Engine, defect)
Core
JavaScript Engine
Tracking
()
RESOLVED
FIXED
mozilla46
Tracking | Status | |
---|---|---|
firefox46 | --- | fixed |
People
(Reporter: luke, Assigned: luke)
Details
Attachments
(1 file, 2 obsolete files)
159.90 KB,
patch
|
bbouvier
:
review+
|
Details | Diff | Splinter Review |
![]() |
Assignee | |
Comment 1•9 years ago
|
||
Attachment #8702672 -
Flags: review?(benj)
![]() |
Assignee | |
Comment 2•9 years ago
|
||
Fixup test outside tests/asm.js.
Attachment #8702672 -
Attachment is obsolete: true
Attachment #8702672 -
Flags: review?(benj)
Attachment #8702675 -
Flags: review?(benj)
![]() |
Assignee | |
Comment 3•9 years ago
|
||
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 4•9 years ago
|
||
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+
![]() |
Assignee | |
Comment 5•9 years ago
|
||
Agreed on all points, thanks!
Comment 7•9 years ago
|
||
bugherder |
Status: ASSIGNED → RESOLVED
Closed: 9 years ago
status-firefox46:
--- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla46
You need to log in
before you can comment on or make changes to this bug.
Description
•