MaolmeoX opened a new pull request, #527: URL: https://github.com/apache/arrow-js/pull/527
## What's Changed `Vector`, `Table` and `RecordBatch` set `Symbol.isConcatSpreadable` on their prototypes when the module loads. V8 keeps a single process-wide protector for that symbol: once any object defines it, whatever its value, every `Array.prototype.concat` in the process takes the slow path for the life of the process. Loading `apache-arrow` therefore made `concat` about 100× slower everywhere, including in code that never touches an Arrow object (see #525). This PR removes the three assignments and the matching `[Symbol.isConcatSpreadable]: true` declarations in the `Vector`, `Table` and `RecordBatch` interfaces. No code in `src/` passes an Arrow object to `Array.prototype.concat`: the remaining `concat` calls concatenate arrays of `Data` or are the classes' own `concat` methods. ### Performance The reproduction script from #525 (20,000 `acc = acc.concat([i])` calls, run before and after `require`) against the ES2015 CommonJS build, median of 3 runs: | Node.js | Build | Before `require` | After `require` | `%IsConcatSpreadableProtector()` after `require` | | --- | --- | ---: | ---: | --- | | 20.20.2 | `main` | 471 ms | 48,150 ms | `false` | | 20.20.2 | this PR | 465 ms | 479 ms | `true` | | 24.19.0 | `main` | 412 ms | 47,410 ms | `false` | | 24.19.0 | this PR | 401 ms | 432 ms | `true` | ### Behaviour change `Array.prototype.concat` now appends an Arrow object as a single element, as for any other object. The symbol never made `concat` spread Arrow objects, because `concat` reads `length` and indexed properties: | Expression | Before | After | | --- | --- | --- | | `[0].concat(vectorFromArray([1, 2, 3]))` | `[0, <3 empty items>]` | `[0, vector]` | | `[0].concat(table)` | `[0]` | `[0, table]` | | `[0].concat(table.batches[0])` | `[0]` | `[0, recordBatch]` | `[...vector]` and `vector.toArray()` give the values. ### Tests `test/unit/concat-tests.ts` checks, for a numeric `Vector`, a `Utf8` `Vector`, a `Table` and a `RecordBatch`, that the object does not define `Symbol.isConcatSpreadable` and that `[0].concat(x)` returns `[0, x]`. All 8 cases fail without the fix. | Check | Node.js | Result | | --- | --- | --- | | Source suite (`jestconfigs/jest.src.config.js`) | 20.20.2 | 50 suites passed | | Source suite (`jestconfigs/jest.src.config.js`) | 24.19.0 | 50 suites passed | | ES2015 CommonJS build (`npm test -- -t es2015 -m cjs`) | 24.19.0 | 50 suites passed | | `npm run lint:ci` | 24.19.0 | clean | Closes #525. -- This is an automated message from the Apache Git Service. To respond to the message, please log on to GitHub and use the URL above to go to the specific comment. To unsubscribe, e-mail: [email protected] For queries about this service, please contact Infrastructure at: [email protected]
