Bug on array spread operator on NodeJS 6.10.0 with --optimize_for_size

[vitorbaptista]

• Version: v6.10.0 and v6.9.5
• Platform: Linux sager 4.4.0-64-generic Update README.md #85-Ubuntu SMP Mon Feb 20 11:50:30 UTC 2017 x86_64 x86_64 x86_64 GNU/Linux

The following code causes an error:

'use strict';
const assert = require('assert');
const entities = Array.apply(null, { length: 1000 }).map(() => (
  {}
));

const bulkBody = entities.reduce((res, entity) => {
  const action = {
    foo: {
      bar: 10,
      baz: 20,
      foobar: undefined,
    },
  };

  // no-op
  if (false) {
    let a;
    let b;
  }

  return [
    ...res,
    action,
    entity,
  ];
}, []);

assert(bulkBody.length == 2 * entities.length, `bulkBody.length: ${bulkBody.length}\tentities.length: ${entities.length}`);

Note that on each loop of the reduce we add 2 elements to the result array. As it starts as an empty array (i.e. with 0 elements), at the end we expect that bulkBody.length == 2 * entities.length. This isn't what happens. See:

$ node --version
v6.10.0
$ node error/reproduce.js  # no errors
$ node --optimize_for_size error/reproduce.js 

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: bulkBody.length: 1999   entities.length: 1000
    at Object.<anonymous> (/home/vitor/Projetos/okfn/opentrials/api/error/reproduce.js:29:1)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3
$ nvm use 5.6.0
Now using node v5.6.0 (npm v3.6.0)
$ node --version
v5.6.0
$ node error/reproduce.js 
$ node --optimize_for_size error/reproduce.js

I tested both on 6.10.0 and 6.9.5 and the error is the same. Note that the code has many no-op operations. If you remove the if (false) {} clause, for example, the error isn't triggered. If you change the entities array length from 1000 to 10000 (for example), the error isn't triggered either. Even if you change the foobar: undefined to foobar: 30, the error isn't triggered.

It seems like that Node is optimizing a very specific code and, if we change even no-op code, the bug isn't triggered.

[fernandobrito]

I could reproduce it on:

• Version: v6.10.0
• Platform: Linux brito-laptop 3.19.0-73-generic fs: fix fd leak on early readstream destroy #81-Ubuntu SMP Tue Oct 18 16:03:37 UTC 2016 x86_64 x86_64 x86_64 GNU/Linux

$ node --version
v6.10.0

$ node reproduce.js # no error
$ node --optimize_for_size reproduce.js                                                                                                                                                           

assert.js:85
  throw new assert.AssertionError({
  ^
AssertionError: bulkBody.length: 1999   entities.length: 1000
    at Object.<anonymous> (/tmp/reproduce.js:29:1)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:394:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:509:3

v7.6.0 seems to be unaffacted.

$ node --version
v7.6.0
$ node reproduce.js                                                                                                                                                                               
$ node --optimize_for_size reproduce.js

[mscdex]

/cc @nodejs/v8

[bnoordhuis]

Interesting bug. I can reproduce and a quick investigation suggests it might have something to do with allocation site pretenuring in Crankshaft.

--trace_deopt logs a rather suspicious reason: allocation-site-tenuring-changed message and adding either --noallocation_site_pretenuring or --nocrankshaft makes the test pass reliably again.

It sometimes passes without the flags too so there is a timing aspect to it as well but I'm reasonably sure allocation site pretenuring is involved somehow.

[targos]

I can't reproduce with d8 (5.1.281.75)

[targos]

@nodejs/v8 FYI I built several 5.x-lkgr V8 branches on 64bit Linux to be able to check easily which release fixes a particular issue. Binaries are here: https://github.com/targos/d8/tree/master/bin

[Trott]

Is this still a bug in Node.js 6.11.1? (Bug isn't tripped on my operating system, so I'm guessing it's Linux-specific.)

[bnoordhuis]

I can still reproduce, on MacOS too. Try running it in a loop:

$ for i in {0..99}; do ./tmp/node-v6.11.1-darwin-x64/bin/node --optimize_for_size tmp/bug11545.js || break; done

assert.js:81
  throw new assert.AssertionError({
  ^
AssertionError: bulkBody.length: 1999   entities.length: 1000
    at Object.<anonymous> (/Users/bnoordhuis/src/v1.x/tmp/bug11545.js:29:1)
    at Module._compile (module.js:570:32)
    at Object.Module._extensions..js (module.js:579:10)
    at Module.load (module.js:487:32)
    at tryModuleLoad (module.js:446:12)
    at Function.Module._load (module.js:438:3)
    at Module.runMain (module.js:604:10)
    at run (bootstrap_node.js:389:7)
    at startup (bootstrap_node.js:149:9)
    at bootstrap_node.js:504:3

[apapirovski]

Works as expected 6.14.0 so I guess this did get fixed at some point.