Leaking of __empty variable and highly inefficient optimization

[trueadm]

Whilst working with React children, one of the operations we do is "flatten" arrays. This is a very common use-case in JS applications and it seems to be one that Prepack struggles to do when the abstract contains abstract elements/lengths.

Take the below case:

(function() {
  function makeArr(x) {
    var arr = [1, 2, 3];
    if (x) {
      arr.push(4);
    } else {
      arr.pop();
    }
    return arr;
  }

  function recursivelyFlatten(arr, baseArr) {
    for (var i = 0; i < arr.length; i++) {
      var elem = arr[i];

      if (Array.isArray(elem)) {
        recursivelyFlatten(elem, baseArr);
      } else {
        baseArr.push(elem);
      }
    }
  }

  function flatten(arr) {
    var baseArr = [];
    recursivelyFlatten(arr, baseArr);
    return baseArr;
  }

  global.fn = function fn(x) {
    var arr1 = makeArr(x);
    var arr2 = makeArr(x);
    return flatten([arr1, arr2]);
  };

  global.__optimize && __optimize(fn);
})();

The output for this is:

(function () {
  var _$Q = this;

  var _1 = function (x) {
    var __get_scope_binding_0 = function (__selector) {
      var __captured;

      switch (__selector) {
        case 0:
        case 1:
          __captured = [void 0, void 0];
          break;
      }

      __scope_0[__selector] = __captured;
      return __captured;
    };

    var __scope_0 = new Array(2);

    var __leaked_6, __leaked_7, __leaked_4, __leaked_5;

    var _W = function () {
      var __captured__scope_1 = __scope_0[0] || __get_scope_binding_0(0);

      for (__captured__scope_1[0] = 0; __captured__scope_1[0] < __leaked_6.length; __captured__scope_1[0]++) {
        __captured__scope_1[1] = __leaked_6[__captured__scope_1[0]];

        if (Array.isArray(__captured__scope_1[1])) {
          _Z(__captured__scope_1[1], __leaked_7);
        } else {
          __leaked_7.push(__captured__scope_1[1]);
        }
      }
    };

    var _X = function () {
      var __captured__scope_2 = __scope_0[1] || __get_scope_binding_0(1);

      for (__captured__scope_2[0] = 0; __captured__scope_2[0] < __leaked_4.length; __captured__scope_2[0]++) {
        __captured__scope_2[1] = __leaked_4[__captured__scope_2[0]];

        if (Array.isArray(__captured__scope_2[1])) {
          _Z(__captured__scope_2[1], __leaked_5);
        } else {
          __leaked_5.push(__captured__scope_2[1]);
        }
      }
    };

    var _2 = [1, 2,,,];
    _2.length = x ? 4 : 2;
    _2[0] = 1;
    _2[1] = 2;

    var _5 = x ? 3 : __empty;

    if (x) _2[2] = 3;else delete _2[2];

    var _9 = x ? 4 : __empty;

    if (x) _2[3] = 4;else delete _2[3];
    __leaked_6 = _2;
    var _I = [,,];
    _I.length = 0;
    delete _I[0];
    delete _I[1];
    __leaked_7 = _I;

    var _$0 = _W();

    var _M = [1, 2,,,];
    _M.length = x ? 4 : 2;
    _M[0] = 1;
    _M[1] = 2;
    if (x) _M[2] = 3;else delete _M[2];
    if (x) _M[3] = 4;else delete _M[3];
    __leaked_4 = _M;
    __leaked_5 = _I;

    var _$5 = _X();

    return _I;
  };

  var _Z = function (arr, baseArr) {
    for (var i = 0; i < arr.length; i++) {
      var elem = arr[i];

      if (Array.isArray(elem)) {
        _Z(elem, baseArr);
      } else {
        baseArr.push(elem);
      }
    }
  };

  var __empty = {};
  _$Q.fn = _1;
}).call(this);

You can see we have __empty serialized out above, which accordingly to @NTillmann, should never happen.

The output that Prepack creates is not optimal at all and definitely will hit the slow paths in all JS VMs. Ideally what we should be outputting in the base-case is this:

function _1(x) {
  var _$1 = x ? [3, 4] : [];
  var _$2 = x ? [3, 4] : [];
  return [1, 2, ..._$0, 1, 2, ..._$1];
}

[NTillmann]

In your example above, the __empty never gets built into a value that is reachable outside of Prepacked code --- so this is all working alright. (Well, if you look very closely, you can see that all uses of __empty are in fact completely dead, which is the result of a peep-hole optimization; suboptimal, but not wrong.)

So I wonder what the actual problem is?

[trueadm]

@NTillmann Oh, I think I misunderstood you then. I thought you said that __empty should never be visible in the output code. In terms of my original problem, the React reconciler is passed a conditional that has one side that is an EmptyValue. Furthermore, the React reconciler in this particular use-case is trying to serialize this conditional, which is an element in array as JSX children. When we do this, it gets output like this:

var conditionalElement = x ? 1 : __empty;
var reactElement = <div>{conditionalElement}{conditionalElement}{conditionalElement}</div>

This is not right though, as the __empty value is now reachable. We can't use Prepack's way of generating arrays because ReactElement children must be explicit, passing in a reference to an array is a correctness issue. Technically, you can get around this by using createElement rather than JSX, where this would work and it would use the existing logic that doesn't expose __empty:

var arr = [1];
if (x) arr[1] = 1;
var reactElement = React.createElement("div", null, ...arr);

Unfortunately, you cannot spread JSX children (it's explicitly forbidden in the spec). So we have to inline each element of the array into the ReactElement, which results in exposing __empty again. I actually have a way of getting around this in my PR (if you look at the latest comments/commits) that this issue is linked to, but this issue is also coverage for why we generate so much bad code anyway (which is never going to give performance wins).