[Perf] set Symbol.isConcatSpreadable only when required

[infinity-naveen]

Context:
Issue: #2

Changes done in PR:

Since, we were facing issue on setting Symbol.isConcatSpreadable on initialisation, we just move this statement inside safeArrayConcat function & execute only when we get any value on which this symbol was already set. Since, reference is passed in callBind.apply(), setting symbol key later after apply() should also make working of $concatApply as expected.

Steps to verify:

executed npm test
Existing 9 test cases were executed and all were passed.
Below code snippet was also executed:

const loadPackage = process.env.loadPackage==="true" || false;
const count = process.env.count || 100;

if (loadPackage) {
    require('safe-array-concat');
}

const runScript = () => {
    console.log(`Running test`);

    // creating sample array of 50k size with dummy data
    const sampleArray = [];
    const arraySize = 1000*50;
    for (let i = 0; i<arraySize; i++) sampleArray.push(i);

    // Actual test:
    console.time("concatOperation");
    for (let i =0; i<count; i++) {
        sampleArray.concat(sampleArray); // <--- Testing impact on this operation's performance
    }
    console.timeEnd("concatOperation");
}

runScript();

Test execution results::
loadPackage=true count=200 node safe-test.js :: 114 ms
loadPackage=false count=200 node safe-test.js :: 113 ms
loadPackage=true count=500 node safe-test.js :: 268 ms
loadPackage=false count=500 node safe-test.js :: 275 ms
(No impact of requiring package on our application now)

[ljharb]

Is the performance issue caused by any assignment of Symbol.isConcatSpreadable?

If we delete empty[Symbol.isConcatSpreadable], does that restore the performance?

[infinity-naveen]

Is the performance issue caused by any assignment of Symbol.isConcatSpreadable?
Yes

If we delete empty[Symbol.isConcatSpreadable], does that restore the performance?

No, that is the issue in V8, if we ever set this symbol once in code, it will impact whole subsequent functionality of concat(). Deleting it will also not restore performance.

[ljharb]

wow, that's horrific. I'll try to poke some v8 folks about it.

this seems viable, then.

[infinity-naveen]

In provided sample script in description, instead of requiring safe-concat-array package, you can just use below line & test:

[][Symbol.isConcatSpreadable] = true; // false value will also impact

This will also impact performance. Any further mutation in array/value will not restore performance.

[infinity-naveen]

Also, if you try same code in Safari javascript console, it will not impact performance (since, Safari uses different Javascript engine, V8 is used in NodeJs & Chrome)

[codecov]

Codecov Report

Patch coverage: 100.00% and no project coverage change.

Comparison is base (f929a1b) 100.00% compared to head (f1c5c9f) 100.00%.

Additional details and impacted files

@@            Coverage Diff            @@
##              main        #3   +/-   ##
=========================================
  Coverage   100.00%   100.00%           
=========================================
  Files            1         1           
  Lines           23        23           
  Branches         9         9           
=========================================
  Hits            23        23           

Files Changed	Coverage Δ
index.js	100.00% <100.00%> (ø)

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[infinity-naveen]

That performance cliff in v8, is expected and trade-off to support @@isConcatSpreadable
Ref: https://bugs.chromium.org/p/v8/issues/detail?id=14299#c3

[ljharb]

It may be expected by v8 devs, but it’s not expected by anyone else :-) a slowdown on operations involving things that set the symbol is expected; a global permanent slowdown on unrelated operations just because it’s assigned anywhere is not.