Closed Bug 1054066 Opened 5 years ago Closed 5 years ago

OdinMonkey: issue deprecation warning for heaps less than 64kb

Categories

(Core :: JavaScript Engine: JIT, defect)

defect
Not set

Tracking

()

RESOLVED FIXED
mozilla34

People

(Reporter: luke, Assigned: luke)

Details

Attachments

(2 files, 1 obsolete file)

Attached patch 64k (obsolete) — Splinter Review
As discussed in
  http://discourse.specifiction.org/t/increase-minimum-heap-length-to-64kb/564
we want to increase the minimum heap size to 64kb.  For now, issue a deprecation warning and later we can make it a link-time validation error.
Attachment #8473321 - Flags: review?(dtc-moz)
Attached patch 64kSplinter Review
It's been a bad day for qref'ing before attaching.
Assignee: nobody → luke
Attachment #8473321 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #8473321 - Flags: review?(dtc-moz)
Attachment #8473351 - Flags: review?(dtc-moz)
Comment on attachment 8473351 [details] [diff] [review]
64k

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

Looks good, thank you.

::: js/src/jit-test/tests/asm.js/testHeapAccess.js
@@ +257,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u16[2047] = -1 } return f'), this, null, buf)();

These are probably to test the bounds checking so u16[2047] => u16[BUF_MIN/2-1]

@@ +262,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u16[2048] = -1 } return f'), this, null, buf)();

u16[BUF_MIN/2]

@@ +279,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { i16[2047] = -1 } return f'), this, null, buf)();

and here

@@ +284,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { i16[2048] = -1 } return f'), this, null, buf)();

and here

@@ +301,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u32[1023] = -1 } return f'), this, null, buf)();

BUF_MIN/4-1

@@ +306,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u32[1024] = -1 } return f'), this, null, buf)();

BUF_MIN/4

@@ +323,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { i32[1023] = -1 } return f'), this, null, buf)();

here

@@ +328,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { i32[1024] = -1 } return f'), this, null, buf)();

here

@@ +345,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { f32[1023] = -1.0 } return f'), this, null, buf)();

here

@@ +350,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { f32[1024] = -1.0 } return f'), this, null, buf)();

here

@@ +367,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { f64[511] = -1.0 } return f'), this, null, buf)();

here

@@ +372,1 @@
>  asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { f64[512] = -1.0 } return f'), this, null, buf)();

here

@@ +416,4 @@
>  new Float32Array(buf)[1] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f32[1]; } return f'), this, null, buf)(),-1.0);
>  new Float32Array(buf)[1023] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f32[1023]; } return f'), this, null, buf)(),-1.0);

here

@@ +417,5 @@
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f32[1]; } return f'), this, null, buf)(),-1.0);
>  new Float32Array(buf)[1023] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f32[1023]; } return f'), this, null, buf)(),-1.0);
>  new Float32Array(buf)[1024] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f32[1024]; } return f'), this, null, buf)(),-1.0);

here

@@ +428,4 @@
>  new Float64Array(buf)[1] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f64[1]; } return f'), this, null, buf)(),-1.0);
>  new Float64Array(buf)[511] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f64[511]; } return f'), this, null, buf)(),-1.0);

here

@@ +429,5 @@
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f64[1]; } return f'), this, null, buf)(),-1.0);
>  new Float64Array(buf)[511] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f64[511]; } return f'), this, null, buf)(),-1.0);
>  new Float64Array(buf)[512] = -1.0;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return +f64[512]; } return f'), this, null, buf)(),-1.0);

here

@@ +441,4 @@
>  new Uint8Array(buf)[8191] = -1;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return u8[8191&8191]|0; } return f'), this, null, buf)(),255);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return u8[(8191&8191)>>0]|0; } return f'), this, null, buf)(),255);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u8[8192&8191] = -1; u8[0] = 0; return u8[8192&8191]|0; } return f'), this, null, buf)(),0);

Probably want make this BUF_MIN&(BUF_MIN-1)

@@ +441,5 @@
>  new Uint8Array(buf)[8191] = -1;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return u8[8191&8191]|0; } return f'), this, null, buf)(),255);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return u8[(8191&8191)>>0]|0; } return f'), this, null, buf)(),255);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u8[8192&8191] = -1; u8[0] = 0; return u8[8192&8191]|0; } return f'), this, null, buf)(),0);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u8[(8192&8191)>>0] = -1; u8[0] = 0; return u8[(8192&8191)>>0]|0; } return f'), this, null, buf)(),0);

and here

@@ +445,5 @@
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { u8[(8192&8191)>>0] = -1; u8[0] = 0; return u8[(8192&8191)>>0]|0; } return f'), this, null, buf)(),0);
>  new Int8Array(buf)[8191] = -1;
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return i8[8191&8191]|0; } return f'), this, null, buf)(),-1);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { return i8[(8191&8191)>>0]|0; } return f'), this, null, buf)(),-1);
>  assertEq(asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + HEAP_IMPORTS + 'function f() { i8[8192&8191] = -1; i8[0] = 0; return i8[8192&8191]|0; } return f'), this, null, buf)(),0);

here, and below

::: js/src/jit-test/tests/asm.js/testZOOB.js
@@ +57,2 @@
>      var f = asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + 'var arr=new glob.' + ctor.name + '(b); function f(i) {i=i|0; return arr[((i<<' + scale + ')+' + disp + ')>>' + shift + ']|0 } return f'), this, null, ab);
> +    for (var i of [0,1,2,3,4,1023,1024,1025,BUF_MIN-2,BUF_MIN-1,BUF_MIN])

perhaps also BUF_MIN+1

@@ +88,2 @@
>      var f = asmLink(asmCompile('glob', 'imp', 'b', USE_ASM + 'var arr=new glob.' + ctor.name + '(b); var toF = glob.Math.fround; function f(i) {i=i|0; return ' + coercion + '(arr[((i<<' + scale + ')+' + disp + ')>>' + shift + ']) } return f'), this, null, ab);
> +    for (var i of [0,1,2,3,4,1023,1024,1025,BUF_MIN-2,BUF_MIN-1,BUF_MIN])

and here
Attachment #8473351 - Flags: review?(dtc-moz) → review+
Attached patch rm-const-pathSplinter Review
Updating all the testcases for the HEAPN[constant>>k] cases reminded me that I really don't think we need this special case.  Removing this special-case shows no decrease in the number of heap accesses that ultimately get marked as skipBoundsCheck.  That leaves only the "it generates less IR argument".  In all large apps I could measure, this optimization hits for <3% of heap accesses, so I don't think we're getting any measurable compile-time improvements either.  Plus, we should fix Emscripten to emit constant heap accesses for these cases anyways (notified Dan and Alon).
Attachment #8473757 - Flags: review?(dtc-moz)
(In reply to Luke Wagner [:luke] from comment #3)
> Created attachment 8473757 [details] [diff] [review]
> rm-const-path
> 
> Updating all the testcases for the HEAPN[constant>>k] cases reminded me that
> I really don't think we need this special case.  Removing this special-case
> shows no decrease in the number of heap accesses that ultimately get marked

Could you give me a little time to explore this one. Some thoughts:

* Yes, emscripten can fold these.

* b[c >> 0] is semantically different to b[c]. Only the later restricts the heap length. Perhaps it's not so important to optimize their use though.

* There may still be a use for the b[c>>n] pattern because the module const could be a hint to store the value in the global data and it might be more useful in some cases to store the pre-shifted-and-shifted-back value (the index that the load instruction needs without shifting). The const declarations could become a way for the code writer to communicate hot constants that can be usefully stored in the global data, and the hottest could be placed first. Perhaps there will be PGO of asm.js someday to order them and make the decision to use a const or a literal. If these constants are stored in the global data then it would be more useful to store the final constant index that is used by the instructions without shifting which would be 'c' in b32[c >> 2] and not the 'c' in b32[c].

Shall follow up.
(In reply to Douglas Crosher [:dougc] from comment #4)
None of these points seem like reasons for keeping the current code as this is all outside the asm.js spec, so just optimizations we can add and remove as necessary.

> * There may still be a use for the b[c>>n] pattern because the module const
> could be a hint to store the value in the global data

I doubt we'd ever be able to do that since dynamic heap accesses could always alias the same location.
Comment on attachment 8473757 [details] [diff] [review]
rm-const-path

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

Have convinced myself that the semantic difference between b[c>>n] and b[c] is not important - emscripten can detect known out-of-bounds arrays accesses and fold them anyway so I don't think b[c>>n] is needed for the semantic difference.

However I still see merit in keeping b[c>>n] as a well optimized pattern that code writers can use without loss of performance. For example, for

const addr1 = 0x10011008;
...
b32[addr1>>2]

An asmjs compiler might decide to place the constant 0x10011008 in a global data slot and load it from there rather than inlining it. The ARMv6 needs to store many integer constants in a pool and might decide to do so in this case. A stored scaled index is no help to the compiler. The asm.js compiler does not know which constants are used more frequently so can not decide how to order them in optimal positions in the global data (positions with a small offset that allow loading in a single instruction), but the code writer can make this decision so it would be natural for an asm.js compiler to order constants in the order they appear in the source code.

It can also help writing readable asm.js code to be able to declare const addresses and still be able to use them efficiently, rather than scaled addresses. Some people might want to write readable hand optimized asm.js code.

But it's your call and the patch looks ok if you want to make this change.
Attachment #8473757 - Flags: review?(dtc-moz) → review+
As long as they're consts, the range analysis should eliminate the bounds checks, so I think we're good to remove:
https://hg.mozilla.org/integration/mozilla-inbound/rev/57c6d6fd6bb1
https://hg.mozilla.org/integration/mozilla-inbound/rev/6b9c89464dc6
https://hg.mozilla.org/mozilla-central/rev/57c6d6fd6bb1
https://hg.mozilla.org/mozilla-central/rev/6b9c89464dc6
Status: ASSIGNED → RESOLVED
Closed: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla34
You need to log in before you can comment on or make changes to this bug.