1077945 - IonMonkey: improve range information for const variables.

Status: NEW

(Core :: JavaScript Engine: JIT, defect, P3)

Description

[Douglas Crosher [:dougc]]

Bug 915157 is exploring the following asm.js pattern:

function test (glob, env, b) {
    'use asm';
    var i32 = new glob.Int32Array(b);
    const mask = 0x7fc;
    function f(i, v) {
...    i32[(i & mask) + 8 >> 2] = v;

For practical reasons this code needs to perform well in Ion, not just Odin. If the range of the constant variable 'mask' were propagate to the heap access code then this could be used to help optimize this code.

Comment 1

[Dan Gohman [:sunfish]]

Ion does optimize 'mask' if the 'const mask = 0x7fc' is moved out to the global scope. Is there something different in the JS semantics about a const in an enclosing function scope versus the global scope?

Comment 2

[Douglas Crosher [:dougc]]

This issue seems to depend on the JS code and when tested on a zlib Emscripten generated file, with index masking, the constant is recognized and propagated. So it might not be such a show stopper if it only affects small test code. Shall keep an eye on it.

Comment 3

[Douglas Crosher [:dougc]]

This is still blocking the use a const in asm.js modules that need to also perform well when compiled in Ion. Without the type info for these constants it is not possible to optimize away the bounds check or the low bit mask, and this in turn blocks dehoisting the constant index offset.

Here are some of the nodes leading up to the v57 argument to a bitand operation which should be a constant.

[102,103 LoadFixedSlotV] [def v55<t>] [def v56<p>] [use v31:r]
[104,105 Unbox] [def v57<i>:tied(0)] [use v56:r?] [use v55:r?] [use v31:*] [use v32:*] [use v33:*] [use v34:*]
[166,167 BitOpI] [def v80<i>:tied(0)] [use v57:r?] [use v67:r?] <<< v57 is a const
[168,169 AddI] [def v81<i>:tied(0)] [use v80:r?] [use c]   <<< small index offset, to be dehoisted
[170,171 BitOpI] [def v82<i>:tied(0)] [use v81:r?] [use c] <<< low bit mask, to be eliminated.
[172,173 StoreTypedArrayElementStatic] [use v82:r] [use v33:r]

Moving the const definitions out of the asm.js module does seem to fix the issue, but this would not be compatible with asm.js code.

Any idea if this is an Ion issue, or might it need some changes at earlier stages to improve this?

fwiw This same pattern is not handler well by v8, same issue, and this would also need fixing for this pattern to be useful to app developers.

Comment 4

[Nicolas B. Pierron [:nbp]]

(In reply to Douglas Crosher [:dougc] from comment #3)
> Any idea if this is an Ion issue, or might it need some changes at earlier
> stages to improve this?
> 
> fwiw This same pattern is not handler well by v8, same issue, and this would
> also need fixing for this pattern to be useful to app developers.

I think we have 3 issues here:
 - Alias Analysis alias the scope chain with any other object.
 - Alias Analysis does not consider non-configurable & non-writable slots as being equivalent to a constant.
 - We do not know if the module was ran only once, and if we can bake-in the constant into the code produced by IonMonkey. (because the constant value could have been computed)

Comment 5

[Douglas Crosher [:dougc]]

(In reply to Nicolas B. Pierron [:nbp] from comment #4)
> (In reply to Douglas Crosher [:dougc] from comment #3)
> > Any idea if this is an Ion issue, or might it need some changes at earlier
> > stages to improve this?
> > 
> > fwiw This same pattern is not handler well by v8, same issue, and this would
> > also need fixing for this pattern to be useful to app developers.
> 
> I think we have 3 issues here:
>  - Alias Analysis alias the scope chain with any other object.
>  - Alias Analysis does not consider non-configurable & non-writable slots as
> being equivalent to a constant.
>  - We do not know if the module was ran only once, and if we can bake-in the
> constant into the code produced by IonMonkey. (because the constant value
> could have been computed)

Thank you.

Does the JIT have any support to specialize on non-changing closure variables? This might be of more general utility.

With such support it might even be possible for the masks to be variables that are changed when the heap length changes. Such a pattern would support faster heap access than Odin with a changing heap!

Comment 6

[Jan de Mooij [:jandem]]

(In reply to Dan Gohman [:sunfish] from comment #1)
> Ion does optimize 'mask' if the 'const mask = 0x7fc' is moved out to the
> global scope. Is there something different in the JS semantics about a const
> in an enclosing function scope versus the global scope?

The global is a singleton object and Ion can bake in properties of those objects (and invalidate JIT code when they change).

Douglas: do all browsers Emscripten cares about support const? Is Ion's performance really a showstopper? We can look into improving it but const-support in other engines is pretty new so we haven't bothered optimizing it yet.

Comment 7

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Dan Gohman [:sunfish] from comment #1)
> > Ion does optimize 'mask' if the 'const mask = 0x7fc' is moved out to the
> > global scope. Is there something different in the JS semantics about a const
> > in an enclosing function scope versus the global scope?
> 
> The global is a singleton object and Ion can bake in properties of those
> objects (and invalidate JIT code when they change).
> 
> Douglas: do all browsers Emscripten cares about support const? Is Ion's
> performance really a showstopper? We can look into improving it but
> const-support in other engines is pretty new so we haven't bothered
> optimizing it yet.

IE 11 has const support. Apps that need webgl will need IE11 so could usefully depend on const.

Runtimes for the zlib benchmark on x86 with patches to optimize for the index masking pattern by eliminating bounds checks and dehoisting offsets:
 A. Using const for the masks : Ion 66, Odin 38
 B. Using inlined literals    : Ion 46, Odin 38

For app developers writing 'almost asm' code the poor relative performance of Ion may be a show stopper for the use of a const for the mask. But it's not a show stopper for using the index masking pattern. Inlining literals will likely increase the code size and this might frustrate the use of the index masking pattern.

If it were possible to use variables to hold the masks and still have Ion specialize on their value then it could open interesting new performance possibilities - namely using the masking pattern with a changing heap length while still achieving top heap access performance.

Comment 8

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #6)
> (In reply to Dan Gohman [:sunfish] from comment #1)
> > Ion does optimize 'mask' if the 'const mask = 0x7fc' is moved out to the
> > global scope. Is there something different in the JS semantics about a const
> > in an enclosing function scope versus the global scope?
> 
> The global is a singleton object and Ion can bake in properties of those
> objects (and invalidate JIT code when they change).

After looking into this a little it looks like the best possible optimization for now would be to bake in const's initialize to literal numbers.

Would I be correct that Ion can specialize a function to the observed typed but that if a closure is created with two differently initialized const's values then it does not create separate code for each instance?

For example the Ion static typed array specific code for the x86 seems to only work if the asm.js module is only run once, or with the same buffer?

I understand Odin will create a new code object each time a module is linked, and Odin can bake in a const that is initialized with a literal number. Potentially Odin could bake in any immutable closure var or const.

I have not given up on Ion baking in a const initialized to a literal number, but perhaps IonBuilder is not the right place? Might it be better done at the JS bytecode stage?

Are there any cases in which Ion does compile separate closure code objects to specialize the code?

fwiw TurboFan appears to be able to specialize a closure to its context.

Comment 9

[Douglas Crosher [:dougc]]

Attachment: Bake in const variables initialized to numeric literals.

A proof of concept patch. This bakes in the constants when emitting the JS byte-code. This gives a 0.67x run time performance improvement on some challenging asm.js code here.

There is also a good deal of constant folding which appears to be applied at the AST stage. Have not evaluated if it would be more appropriate to bake in the const there.

Also is it known from the AST if a var is written? So that an immutable var initialized to a numeric literal could also have its value baked in?

Any opinion on what range of values it would be appropriate to bake in, if any?

Comment 10

[Douglas Crosher [:dougc]]

Attachment: Bake in const variables initialized to numeric literals.

As above, same questions, but this version handles negative constants. The case of -1 is important for disabling index masking.

Comment 11

[Jan de Mooij [:jandem]]

Comment on attachment 8572433 [details] [diff] [review]
Bake in const variables initialized to numeric literals.

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

Unfortunately we can't bake in consts in the emitter, because the debugger can modify consts. We've been moving optimizations like this out of the frontend for this reason; the JITs know how to deal with the debugger by recompiling/invalidating code etc.

Can you post a small but representative testcase where Ion is not baking in the const atm?

Comment 12

[Douglas Crosher [:dougc]]

Attachment: Example annotated test JS code.

Comment 13

[Douglas Crosher [:dougc]]

(In reply to Jan de Mooij [:jandem] from comment #11)
...
> Unfortunately we can't bake in consts in the emitter, because the debugger
> can modify consts. We've been moving optimizations like this out of the
> frontend for this reason; the JITs know how to deal with the debugger by
> recompiling/invalidating code etc.

Ok, thank you for the feedback.
 
> Can you post a small but representative testcase where Ion is not baking in
> the const atm?

Done. The example code is annotated with the opportunities.

The use case is similar to the need for JS macro support - an ability to make fine grain code changes while compiling. While JS could rewrite the code, it's not clear this could be pipelined with download and compilation, and if such rewriting introduces a significant delay of overhead then it's probably not practically useful. I am exploring if a service-work could do such rewriting, but have no answer yet.

Comment 14

[Shu-yu Guo [:shu]]

(In reply to Jan de Mooij [:jandem] from comment #11)
> Comment on attachment 8572433 [details] [diff] [review]
> Bake in const variables initialized to numeric literals.
> 
> Review of attachment 8572433 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Unfortunately we can't bake in consts in the emitter, because the debugger
> can modify consts. We've been moving optimizations like this out of the
> frontend for this reason; the JITs know how to deal with the debugger by
> recompiling/invalidating code etc.

Jan and I talked about this on IRC. Sounds like we need a new TI constraint onDebuggerMutation that needs to be installed on objects we assume the non-type state (e.g., constness, configurability) of in Ion.

Comment 15

[Jan de Mooij [:jandem]]

Last month I noticed the Debugger can't modify aliased consts but it can modify unaliased ones. I filed bug 1141255 on this; we should probably fix the debugger so it can't modify any consts (similar to how it doesn't let you change non-writable properties).

That makes it more feasible to bake in consts in the emitter or Ion.

Comment 16

[Till Schneidereit [:till]]

As more code begins using consts, this becomes more important.