Skip to content
This repository has been archived by the owner on Feb 12, 2022. It is now read-only.

Crash with --heapGraphFilePath #1732

Closed
NTillmann opened this issue Apr 11, 2018 · 2 comments
Closed

Crash with --heapGraphFilePath #1732

NTillmann opened this issue Apr 11, 2018 · 2 comments

Comments

@NTillmann
Copy link
Contributor

NTillmann commented Apr 11, 2018

Try prepacking this with --heapGraphFilePath:

(function () {
  let a = __abstract("number", "a");
  let b, c;
  global.f = function() {
    b = a + 42;
    c = a + 42;
  }
  __optimize(f);
})();

And without that option, we generate the following dubious code. Note the duplicate var __captured__scope_2 = __scope_0[0] || __scope_1(0); statements, and the lack of CSE for 42 + _3.

(function () {
  var __scope_0 = Array(1);

  var __scope_1 = function (__selector) {
    var __captured;

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

      default:
        throw new Error("Unknown scope selector");
    }

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

  var _0 = function () {
    var __captured__scope_2 = __scope_0[0] || __scope_1(0);

    var __captured__scope_2 = __scope_0[0] || __scope_1(0);

    var _3 = a;

    var _1 = 42 + _3;

    __captured__scope_2[0] = _1;

    var _4 = 42 + _3;

    __captured__scope_2[1] = _4;
  };

  f = _0;
})();
NTillmann added a commit that referenced this issue Apr 11, 2018
Release notes: None

Don't generate redundant calls of the form
`var __captured__scope_2 = __scope_0[0] || __scope_1(0);`

Adding regression test.

This addresses one of the observations of #1732.
NTillmann added a commit that referenced this issue Apr 11, 2018
Release notes: None

Don't generate redundant calls of the form
`var __captured__scope_2 = __scope_0[0] || __scope_1(0);`

Adding regression test.

This addresses one of the observations of #1732.
Also deleted unneeded left-over code and comment that used to refer #989.
facebook-github-bot pushed a commit that referenced this issue Apr 12, 2018
Summary:
Release notes: None

Don't generate redundant calls of the form
`var __captured__scope_2 = __scope_0[0] || __scope_1(0);`

Adding regression test.

This addresses one of the observations of #1732.
Also deleted unneeded left-over code and comment that used to refer #989.
Closes #1734

Differential Revision: D7602949

Pulled By: NTillmann

fbshipit-source-id: 77d926b236a903eb7927a1471411e8397a0d192a
trueadm pushed a commit to trueadm/prepack that referenced this issue Apr 13, 2018
Summary:
Release notes: None

Don't generate redundant calls of the form
`var __captured__scope_2 = __scope_0[0] || __scope_1(0);`

Adding regression test.

This addresses one of the observations of facebookarchive#1732.
Also deleted unneeded left-over code and comment that used to refer facebookarchive#989.
Closes facebookarchive#1734

Differential Revision: D7602949

Pulled By: NTillmann

fbshipit-source-id: 77d926b236a903eb7927a1471411e8397a0d192a
@NTillmann
Copy link
Contributor Author

The the duplicate var __captured__scope_2 = __scope_0[0] || __scope_1(0); statements, and the lack of CSE for 42 + _3 have been addressed by now, but the crash remains.

caiismyname pushed a commit that referenced this issue May 24, 2018
facebook-github-bot pushed a commit that referenced this issue Jun 3, 2018
Summary:
Fixing some incompatibility between heapGraph and `__optimize()`.
Essence of the issue was an implicit assumption that only one visitor would visit certain entries.

Original Issue: #1732
Closes #2021

Reviewed By: trueadm

Differential Revision: D8248761

Pulled By: caiismyname

fbshipit-source-id: dd27487b490e3924fdc0fe6aaca7f1c103e137cb
@NTillmann
Copy link
Contributor Author

No longer repros.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

1 participant