Safety analysis for JS template placeholders.

Safety analysis for JS template placeholders.

Determines how many expressions may be substituted for placeholders
without changing the order of the effects embedded in the expressions
or the JavaScript template.

R=asgerf@google.com

Committed: https://github.com/dart-lang/sdk/commit/aebbd86e0ff26be0a7cac1df81eaee1343ab72bc

[sra1, 2015-12-08 04:41:13]

Patchset #1 (id:1) has been deleted

[sra1, 2015-12-08 04:41:46]

sra@google.com changed reviewers:
+ asgerf@google.com

[asgerf, 2015-12-08 11:40:37]

LGTM if integrated to actually be used by at least one backend.

I tried it on swarm using this code in statement_rewriter, feel free to patch
that in:

  void handleForeignCode(ForeignCode node) {
    // Arguments will get inserted in a JS code template.  The arguments will
    // not always be evaluated (e.g. if the template is '# && #').
    bool isNullable(e) => true;
    int safeArguments =
      PlaceholderSafetyAnalysis.analyze(node.codeTemplate.ast, isNullable);
    inEmptyEnvironment(() {
      for (int i = node.arguments.length - 1; i >= safeArguments; --i) {
        node.arguments[i] = visitExpression(node.arguments[i]);
      }
    });
    for (int i = safeArguments - 1; i >= 0; --i) {
      node.arguments[i] = visitExpression(node.arguments[i]);
    }
  }

It saves 960 KB minified.
If isNullable(e) => false, then the saving goes up to 1041 KB.
If safeArguments=node.arguments.length, then 1295 KB.

Given that the theoretical best is somewhere between 960 and 1295 it seems to be
doing a pretty good job :)

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js/placeholder_safety.dart (right):

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:22: class
PlaceholderSafetyAnalysis extends js.BaseVisitor<NativeThrowBehavior> {
Extends BaseVisitor<NativeThrowBehavior> but all the visits return int.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:67: print(node);
Renegade print statement

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:100: safe = false;
Why safe = false here?

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:125: if (canBeNull(first)) safe
= false;
If the JS fragment is annotated with throws:never we could avoid this. Might we
worth adding the throws annotation as an argument.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:151: // "a.x = b.y" detects a
null `b` before a null `a`.
It seems the relevant information is that the RHS is evaluated before the LHS
throws on a null receiver.  There is nothing here about the RHS itself throwing.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:158: //
Blank comment

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:214: safe = false;
Just an observation.

If there are no placeholders in the right operand there is no need for
everything after to be marked as unsafe. But it doesn't seem to make any
difference for our current set of JS templates.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:249: return NONNULL_VALUE;
The binary operators return UNKNOWN_VALUE. Is there a reason for this
difference?

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:257: default:
What about prefix ++ and --?

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:273: // reference errors, e.g
'Array'.
I think we could justify adding window and self to this list.

[asgerf, 2015-12-08 11:41:11]

On 2015/12/08 11:40:37, asgerf wrote:
> LGTM if integrated to actually be used by at least one backend.
> 
> I tried it on swarm using this code in statement_rewriter, feel free to patch
> that in:
> 
>   void handleForeignCode(ForeignCode node) {
>     // Arguments will get inserted in a JS code template.  The arguments will
>     // not always be evaluated (e.g. if the template is '# && #').
>     bool isNullable(e) => true;
>     int safeArguments =
>       PlaceholderSafetyAnalysis.analyze(node.codeTemplate.ast, isNullable);
>     inEmptyEnvironment(() {
>       for (int i = node.arguments.length - 1; i >= safeArguments; --i) {
>         node.arguments[i] = visitExpression(node.arguments[i]);
>       }
>     });
>     for (int i = safeArguments - 1; i >= 0; --i) {
>       node.arguments[i] = visitExpression(node.arguments[i]);
>     }
>   }
> 
> It saves 960 KB minified.
> If isNullable(e) => false, then the saving goes up to 1041 KB.
> If safeArguments=node.arguments.length, then 1295 KB.
> 
> Given that the theoretical best is somewhere between 960 and 1295 it seems to
be
> doing a pretty good job :)
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> File pkg/compiler/lib/src/js/placeholder_safety.dart (right):
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:22: class
> PlaceholderSafetyAnalysis extends js.BaseVisitor<NativeThrowBehavior> {
> Extends BaseVisitor<NativeThrowBehavior> but all the visits return int.
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:67: print(node);
> Renegade print statement
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:100: safe = false;
> Why safe = false here?
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:125: if (canBeNull(first))
safe
> = false;
> If the JS fragment is annotated with throws:never we could avoid this. Might
we
> worth adding the throws annotation as an argument.
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:151: // "a.x = b.y" detects a
> null `b` before a null `a`.
> It seems the relevant information is that the RHS is evaluated before the LHS
> throws on a null receiver.  There is nothing here about the RHS itself
throwing.
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:158: //
> Blank comment
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:214: safe = false;
> Just an observation.
> 
> If there are no placeholders in the right operand there is no need for
> everything after to be marked as unsafe. But it doesn't seem to make any
> difference for our current set of JS templates.
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:249: return NONNULL_VALUE;
> The binary operators return UNKNOWN_VALUE. Is there a reason for this
> difference?
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:257: default:
> What about prefix ++ and --?
> 
>
https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
> pkg/compiler/lib/src/js/placeholder_safety.dart:273: // reference errors, e.g
> 'Array'.
> I think we could justify adding window and self to this list.

Sorry those numbers were in bytes, not KB.

[sra1, 2015-12-08 20:54:04]

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
File pkg/compiler/lib/src/js/placeholder_safety.dart (right):

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:22: class
PlaceholderSafetyAnalysis extends js.BaseVisitor<NativeThrowBehavior> {
On 2015/12/08 11:40:37, asgerf wrote:
> Extends BaseVisitor<NativeThrowBehavior> but all the visits return int.

Done.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:67: print(node);
On 2015/12/08 11:40:37, asgerf wrote:
> Renegade print statement

Done.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:100: safe = false;
On 2015/12/08 11:40:37, asgerf wrote:
> Why safe = false here?

Not sure why. It would occur only in the body of a function.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:125: if (canBeNull(first)) safe
= false;
On 2015/12/08 11:40:37, asgerf wrote:
> If the JS fragment is annotated with throws:never we could avoid this. Might
we
> worth adding the throws annotation as an argument.

Acknowledged.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:151: // "a.x = b.y" detects a
null `b` before a null `a`.
On 2015/12/08 11:40:37, asgerf wrote:
> It seems the relevant information is that the RHS is evaluated before the LHS
> throws on a null receiver.  There is nothing here about the RHS itself
throwing.

I have clarified the comment (I hope).

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:158: //
On 2015/12/08 11:40:37, asgerf wrote:
> Blank comment

Done.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:214: safe = false;
On 2015/12/08 11:40:37, asgerf wrote:
> Just an observation.
> 
> If there are no placeholders in the right operand there is no need for
> everything after to be marked as unsafe. But it doesn't seem to make any
> difference for our current set of JS templates.

Acknowledged.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:249: return NONNULL_VALUE;
On 2015/12/08 11:40:37, asgerf wrote:
> The binary operators return UNKNOWN_VALUE. Is there a reason for this
> difference?

No reason.  They all return numbers, strings, bools.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:257: default:
On 2015/12/08 11:40:37, asgerf wrote:
> What about prefix ++ and --?

Done.

https://codereview.chromium.org/1510633003/diff/20001/pkg/compiler/lib/src/js...
pkg/compiler/lib/src/js/placeholder_safety.dart:273: // reference errors, e.g
'Array'.
On 2015/12/08 11:40:37, asgerf wrote:
> I think we could justify adding window and self to this list.

Done.

[sra1, 2015-12-08 21:00:37]

Patchset #3 (id:60001) has been deleted

[sra1, 2015-12-08 21:57:22]

Description was changed from

==========
Safety analysis for JS template placeholders.

Determines how many expressions may be substituted for placeholders
without changing the order of the effects embedded in the expressions
or the JavaScript template.
==========

to

==========
Safety analysis for JS template placeholders.

Determines how many expressions may be substituted for placeholders
without changing the order of the effects embedded in the expressions
or the JavaScript template.

R=asgerf@google.com

Committed:
https://github.com/dart-lang/sdk/commit/aebbd86e0ff26be0a7cac1df81eaee1343ab72bc
==========

[sra1, 2015-12-08 21:57:22]

Committed patchset #3 (id:80001) manually as
aebbd86e0ff26be0a7cac1df81eaee1343ab72bc (presubmit successful).