Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

DDC / dart2js inconsistencies around undefined and null #36116

Open
rakudrama opened this issue Mar 5, 2019 · 4 comments
Open

DDC / dart2js inconsistencies around undefined and null #36116

rakudrama opened this issue Mar 5, 2019 · 4 comments
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dart2js web-dev-compiler

Comments

@rakudrama
Copy link
Member

rakudrama commented Mar 5, 2019

There are inconsistencies between how DDC and dart2js use JavaScript's undefined value. The compilers are mostly self-consistent, but this occasionally causes problems, often at the js-interop boundary.

Is there something we can do to make DDC and dart2js more consistent?

A summary of the differences:

DDC tries to always use JavaScript null for Dart null.
This enables undefined to be used as a token that signifies an omitted positional optional argument.
This has the nice property that this is how ES6 optional arguments are defined.
The cost is that places that naturally return undefined need to be coerced to null, e.g. values from ES6 Maps need to be coerced via

return value == null ? null : value;

One of the most expensive coercions is that Arrays need to be filled, i.e.

new List(n)  -->  new Array(n).fill(null)

Occasionally an undefined value leaks out (e.g. #36052). Also see example at end.

dart2js uses both JavaScript undefined and null for Dart null.
The calling convention uses the method name (aka a selector, foo$1 vs foo$2) to encode the optional argument, so there is no sentinel value that can accidentally make a passed argument be defaulted.
Arrays do not need to be filled, and are a major source of undefined values.

new List(n) --> new Array(n)

dart2js is an optimizing compiler and has many code-size reducing transformations enabled by allowing undefined. E.g.

return null;   -->  return;
   -- and can be omitted entirely at the end of a method body

var x = null;            -->  var x;
a == null ? null : a     -->   a          -- coercion is erased
a == null ? null : a.x   -->  a && a.x  -- no longer coerces on null path

These optimizations, taken as a whole, are responsible for a substantial code size reduction, but they make it hard to be sure what exactly is being passed to JavaScript.

Problems in pushing to convergence:

  • Filling Arrays is expensive and makes some benchmarks slower (worst are 1.2x to 6x slower).
  • Can the dart2js optimizations be made context sensitive? That a value flows to JS-interop is an interprocedural property. Our experience with tracing in type inference is that this would be only mildly effective.

The following prints x: null on VM and dart2js and x: hello on DDC.
(The pragmas stop dart2js constant-folding the whole program to print("x: null").)

@pragma('dart2js:noInline')
foo(){}

@pragma('dart2js:noInline')
use([x = 'hello']) {
  print('x: $x');
}

main() {
  use(foo());
}
@matanlurey
Copy link
Contributor

How often is a filled array used in a web program?

@jmesserly
Copy link

How often is a filled array used in a web program?

Rarely, it would seem.

I don't think changing DDC's calling convention is a feasible at this point, but we can fix functions to return null and use .fill(null) on arrays pretty easily.

@jmesserly
Copy link

jmesserly commented Mar 13, 2019

Regarding dart2js, I don't think changing dart2js makes sense if it comes with a perf cost? Unless there are some benefits, like situations where V8 will perform better when it doesn't have to deal with undefined vs null. So my thought is to fix the remaining cases where DDC leaks undefined (as they seem to be rarely encountered in practice).

edit: p.s., awesome bug report. The detail here is excellent. :)

@greglittlefield-wf
Copy link
Contributor

FYI, we just ran into another case where DDC leaks undefined: #36652

@vsmenon vsmenon added the area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. label Jul 20, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area-web Use area-web for Dart web related issues, including the DDC and dart2js compilers and JS interop. web-dart2js web-dev-compiler
Projects
None yet
Development

No branches or pull requests

5 participants