improve Dictionary performance

[alexlamsl]

No description provided.

[alexlamsl]

@kzc this is #5172 (comment) alongside some clean-up.

[kzc]

This will have brutal timings because it creates a temporary Array of keys:

    size: function() {
         return Object.keys(this.values).length;
    },

Can you compare timings against a for/in counting loop that creates no temporaries?

.size() is only used in --output ast

Maybe it doesn't matter if that's the case.

[alexlamsl]

That was my reason for not paying attention, but your suggestion obviously sounds better 👍

[kzc]

If this is the only use of Dictionary.size() in uglify-js:

              return value.size() ? value.map(symdef) : undefined; 

then remove it and replace it with Dictionary.empty():

              return !value.empty() ? value.map(symdef) : undefined; 

which need only iterate a single key.

[kzc]

Dictionary.empty() can be added, but Dictionary.size() has to be kept because I think it is part of the user-exported API.

[kzc]

Can you compare timings against a for/in counting loop that creates no temporaries?

My timings hunch for Dictionary.size() was wrong for node v16 at least...

function obj_size() {
  return Object.keys(obj).length;
}
function obj_size_counter() {
  var c = 0;
  for (var k in obj) ++c;
  return c;
}

obj_size	0.761ms
obj_size_counter	0.865ms

The V8 team must have optimized that path.

[kzc]

This poor timing for obj_empty relative to obj_size is unexpected for node v16:

function obj_empty() {
  for (var k in obj) return false;
  return true;
}

obj_size	0.800ms
obj_size_counter	0.901ms
obj_empty	0.786ms

[kzc]

If you see similar timings on your machine with the latest version of NodeJS, then just keep the .size() implementation in #5202 (comment) and ignore all the suggestions above.

[alexlamsl]

TIL Object.create(null).__proto__ === void 0 on every platform except Node.js v0.10

[kzc]

Made me look:

$ node-v0.10.41 -p 'Object.create(null).__proto__'
null

Does it matter for anything in uglify-js? Perhaps to look up the property "__proto__" in an object map?

[alexlamsl]

On its own it seems harmless, but turns out it is symptomatic of a larger issue:

$ node -v
v0.10.48
$ echo 'var o=Object.create(null); o.__proto__ = 42; for (var k in o) console.log(k);' | node

$ node -v
v16.13.0
$ echo 'var o=Object.create(null); o.__proto__ = 42; for (var k in o) console.log(k);' | node
__proto__

so we can't use "__proto__" on that one particular platform.

[kzc]

so we can't use "proto" on that one particular platform.

You could switch to using the legacy Dictionary implementation if that __proto__ lookup doesn't work, similar to what I did in my patch for Map. It's better than basing the logic on a node version.

[alexlamsl]

Agreed − just pondering if I could minimise code duplication... 🤔

[kzc]

By the way, in testing my original patch against old NodeJS versions I was seeing failures which I initially mistakenly attributed to my patch - but then I tried again without the patch and the failures remained. It appears that you are running the latest minor+patch versions of each NodeJS release in CI testing, whereas my local versions of NodeJS are not the latest of each major release. So some V8 bug fixes are not present in my node versions, and many tests fails as result for node versions 12.x and earlier on my local machine. It's not worth addressing but the solution would be to just raise the node_version in those failing tests. It doesn't matter.

[alexlamsl]

If you don't mind giving me the specific Node versions, I'm gonna catch'em all 👻

[alexlamsl]

... actually, if those test failures you've encountered are due to language "features", I can just try every vXXX.0.0 and that should cover all the bases, right?

Of course if it is some bug you've run into in some intermediate verions, then this methodology won't work...

[kzc]

If you don't mind giving me the specific Node versions, I'm gonna catch'em all 👻

Again, I don't think it's worth the trouble, but here are my local versions of NodeJS:

node-v0.10.41
node-v0.12.9
node-v4.2.1
node-v6.9.0
node-v8.0.0
node-v10.4.1
node-v12.13.0
node-v14.16.0
node-v14.3.0
node-v16.1.0

I think v14 and v16 were fine. Every time a new V8 feature was introduced it took a major version to stabilize.

[kzc]

Of course if it is some bug you've run into in some intermediate verions, then this methodology won't work...

True. No way to know without testing every point release. Don't bother.

[alexlamsl]

I shudder to think how many of these got blamed on uglify-js out in the wild... 😅

[alexlamsl]

Just tested with and without "$" − those .substr(1) do add up to measureable performance degradation in node test/benchmark --toplevel -mc passes=100,pure_getters,unsafe

[alexlamsl]

The V8 team must have optimized that path.

var values = function() {
    var values = new Array(100000);
    for (var i = values.length; --i >= 0;) {
        values[i] = Math.floor(Math.random() * values.length);
    }
    return values;
}();

function test(fn) {
    for (var i = 10; --i >= 0;) {
        fn();
    }
    var start = Date.now();
    for (var i = 100; --i >= 0;) {
        fn();
    }
    console.log(fn.name + "\t" + (1e-3 * (Date.now() - start)).toFixed(3) + "ms");
}

var obj = Object.create(null);
function obj_insert() {
    for (var i = 0; i < values.length; i++) {
        obj[i] = values[i];
    }
}
function obj_keys() {
    return Object.keys(obj).length;
}
function obj_loop() {
    var count = 0;
    for (var k in obj) {
        count++;
    }
    return count;
}
function obj_empty() {
    for (var k in obj) {
        return false;
    }
    return true;
}

test(obj_insert);
test(obj_keys);
test(obj_loop);
test(obj_empty);

Seems to be consistent across versions of v8, except for .empty() which is much faster on earlier versions of Node.js − make any sense to you?

$ node -v
v0.10.48

$ cat test.js | node
obj_insert      0.047ms
obj_keys        0.784ms
obj_loop        1.497ms
obj_empty       0.165ms

$ node -v
v4.9.1

$ cat test.js | node
obj_insert      0.054ms
obj_keys        0.843ms
obj_loop        2.274ms
obj_empty       0.048ms

$ node -v
v8.17.0

$ cat test.js | node
obj_insert      0.105ms
obj_keys        1.056ms
obj_loop        1.175ms
obj_empty       1.076ms

$ node -v
v12.22.7

$ cat test.js | node
obj_insert      0.117ms
obj_keys        0.772ms
obj_loop        0.925ms
obj_empty       0.769ms

$ node -v
v16.13.0

$ cat test.js | node
obj_insert      0.106ms
obj_keys        0.705ms
obj_loop        0.846ms
obj_empty       0.753ms

[kzc]

If I had to guess, it appears when a for/in loop is used in any capacity it will preemptively call Object.keys(obj) due to typical usage patterns they gleaned in the wild. That's probably why iterating once or all elements is basically the same.

[alexlamsl]

Another __proto__ lesson of the day:

$ echo 'var {__proto__:a} = {__proto__:42}; console.log(a);' | node
[Object: null prototype] {}

$ echo 'var {__proto__:a} = {"__proto__":42}; console.log(a);' | node
[Object: null prototype] {}

$ echo 'var {__proto__:a} = {["__proto__"]:42}; console.log(a);' | node
42

$ echo 'var {"__proto__":a} = {__proto__:42}; console.log(a);' | node
[Object: null prototype] {}

$ echo 'var {"__proto__":a} = {"__proto__":42}; console.log(a);' | node
[Object: null prototype] {}

$ echo 'var {"__proto__":a} = {["__proto__"]:42}; console.log(a);' | node
42

$ echo 'var {["__proto__"]:a} = {__proto__:42}; console.log(a);' | node
[Object: null prototype] {}

$ echo 'var {["__proto__"]:a} = {"__proto__":42}; console.log(a);' | node
[Object: null prototype] {}

$ echo 'var {["__proto__"]:a} = {["__proto__"]:42}; console.log(a);' | node
42

[kzc]

I think it's by design to distinguish between the property and the prototype reference. But yeah, the rules governing destructuring are pretty weird.

[alexlamsl]

True to form, Node.js v6.0.0 for instance spew out 💎 like these:

    Running test [mangle_arrow_1]
!!! Invalid input or expected stdout
---INPUT---
{
    var N = 1;
    ((o, {
        pname: p
    } = o, {
        [p + N]: v
    } = o) => {
        let N;
        console.log(v);
    })({
        pname: "x",
        x1: "PASS"
    });
}
---EXPECTED STDOUT---
PASS

---ACTUAL ERROR---
ReferenceError: p is not defined


    Running test [mangle_arrow_1_toplevel]
!!! Invalid input or expected stdout
---INPUT---
{
    var N = 1;
    ((o, {
        pname: p
    } = o, {
        [p + N]: v
    } = o) => {
        let N;
        console.log(v);
    })({
        pname: "x",
        x1: "PASS"
    });
}
---EXPECTED STDOUT---
PASS

---ACTUAL ERROR---
ReferenceError: p is not defined


    Running test [mangle_arrow_2]
!!! Invalid input or expected stdout
---INPUT---
{
    var N = 1;
    (({
        pname: p = "x",
        i: n = N
    }, {
        [p + n]: v
    }) => {
        let N;
        console.log(v);
    })({}, {
        x1: "PASS"
    });
}
---EXPECTED STDOUT---
PASS

---ACTUAL ERROR---
ReferenceError: p is not defined


    Running test [mangle_arrow_2_toplevel]
!!! Invalid input or expected stdout
---INPUT---
{
    var N = 1;
    (({
        pname: p = "x",
        i: n = N
    }, {
        [p + n]: v
    }) => {
        let N;
        console.log(v);
    })({}, {
        x1: "PASS"
    });
}
---EXPECTED STDOUT---
PASS

---ACTUAL ERROR---
ReferenceError: p is not defined

[kzc]

True to form, Node.js v6.0.0 for instance spew out 💎 like these:

Increasing random failures on node-v6 recently? Bound to be all sorts of bugs when they're chasing JIT performance.

[alexlamsl]

In those particular cases I think it's more to do with v6 being the first version with destructuring, so they are just stumbling along the way.

I don't think they cared too much about performance until v10 − at least if they did it wasn't showing up 🤣

[alexlamsl]

Here comes Node.js v8.0.0:

(async function f() {
    return 42 in f();
})();
console.log("PASS");

$ cat issue-4974.js | node
node: src\async-wrap.cc:621: Assertion `(env()->current_async_id()) == (0)' failed.

[kzc]

It makes sense that the first version to have a particular feature to be unstable with that feature.

You might consider bumping the node_version of the tests that tend to fail for a given node version.