Odin: don't set needBoundsCheck from the frontend

RESOLVED FIXED in Firefox 47

Status

()

Core
JavaScript Engine: JIT
RESOLVED FIXED
2 years ago
2 years ago

People

(Reporter: sunfish, Assigned: sunfish)

Tracking

(Blocks: 1 bug)

unspecified
mozilla47
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(firefox47 fixed)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

2 years ago
Currently, the AsmJS parser analyzes heap addresses to see if they are provably within the minimum heap size, and if so, it sets the needsBoundsCheck flag to optimize away their bounds check. However, it'll be simpler to do this within Ion, as this doesn't require a needsBoundsCheck flag in the wasm encoding and decoding layers.
When originally added to the asm.js parser I think that detecting some of these cases when parsing was just a short cut to avoid some intermediate overhead and that the type analysis was capable of the same optimizations anyway. There might have been some pipeline issues that resulted in better code when making the decisions earlier.

Unless all the main wasm runtimes are prepared to include a type analysis stage that does a good job of eliminating the bounds checks then there is the potential for some significant performance differences here, and there has been a lot of resistance to doing this, and some of the reasons mentioned were the complexity of the code and the consequence of bugs.

One possibility is that the wasm binary encoding supports a 'safe' flag on memory accesses, and that it could be ignored when the wasm runtime does not trust the source, and taken advantage of when the runtime does trust it.

If the pipeline allows a trusted user module to check and set these flags then this aspect of the type derivation strategy might to be moved out of the wasm spec and runtimes and into trusted modules in the wasm pipeline - an ecosystem of trusted type analysis and bounds check elimination support code. This would give the user more choice managing the security versus performance trade off, and allow the user to make the same choices across runtimes, and avoid wasm spec'ing the type derivation support expected. Just a thought, even if it does not affect the immediate practical issue at hand.
(Assignee)

Comment 2

2 years ago
Created attachment 8721147 [details] [diff] [review]
asmjs-needs-bounds-checks.patch

dougc: Thanks for the thought. There would of course be some practical security concerns to take into consideration.
Attachment #8721147 - Flags: review?(luke)
Comment on attachment 8721147 [details] [diff] [review]
asmjs-needs-bounds-checks.patch

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

::: js/src/asmjs/WasmGenerator.h
@@ +98,5 @@
>  {
>      CompileArgs                     args;
>      ModuleKind                      kind;
>      uint32_t                        numTableElems;
> +    uint32_t                        minHeapLength;

Can you make this an Atomic<uint32_t>?

::: js/src/jit/MIR.h
@@ +13757,1 @@
>                     unsigned numSimdElems, MemoryBarrierBits before, MemoryBarrierBits after)

Can numSimdElems go on the preceding line?

::: js/src/jit/MIRGenerator.h
@@ +37,5 @@
>    public:
>      MIRGenerator(CompileCompartment* compartment, const JitCompileOptions& options,
>                   TempAllocator* alloc, MIRGraph* graph,
>                   const CompileInfo* info, const OptimizationInfo* optimizationInfo,
> +                 bool usesSignalHandlersForAsmJSOOB, uint32_t minHeapLength);

I feel a little bad having these be ctor args, given that they are asm.js/wasm-specific (and rather ad hoc).  How about putting these two as initX() methods instead?

@@ +163,5 @@
>          return abortedPreliminaryGroups_;
>      }
>  
> +    uint32_t minHeapLength() const {
> +        return minHeapLength_;

Can you put this member (and the field) next to all the other AsmJS* methods in MIRGenerator and maybe put AsmJS in the name.  (I'll do a bulk renaming of AsmJS things at some point, for now all the wasm/asm.js-related ones have AsmJS in the name.)
(Assignee)

Comment 4

2 years ago
Created attachment 8721159 [details] [diff] [review]
asmjs-needs-bounds-checks.patch
Attachment #8721147 - Attachment is obsolete: true
Attachment #8721147 - Flags: review?(luke)
Attachment #8721159 - Flags: review?(luke)
Attachment #8721159 - Flags: review?(luke) → review+
(Assignee)

Updated

2 years ago
Blocks: 1249787

Comment 6

2 years ago
bugherder
https://hg.mozilla.org/mozilla-central/rev/71817a535eff
Status: NEW → RESOLVED
Last Resolved: 2 years ago
status-firefox47: --- → fixed
Resolution: --- → FIXED
Target Milestone: --- → mozilla47
You need to log in before you can comment on or make changes to this bug.