Skip to content

Conversation

@falsandtru
Copy link

@falsandtru
Copy link
Author

🤔

Error: You must provide `username` and `accessKey` as options or set "SAUCE_USERNAME" and "SAUCE_ACCESS_KEY" in your environment variables.
    at runSauceLabs (/home/travis/build/primus/eventemitter3/node_modules/sauce-test/lib/run-sauce-labs.js:46:29)
    at runTestsAtLocation (/home/travis/build/primus/eventemitter3/node_modules/sauce-test/index.js:68:12)
    at publishCode.then.location (/home/travis/build/primus/eventemitter3/node_modules/sauce-test/index.js:140:12)
    at /home/travis/build/primus/eventemitter3/node_modules/promise/lib/core.js:33:15
    at flush (/home/travis/build/primus/eventemitter3/node_modules/promise/node_modules/asap/asap.js:27:13)
    at process._tickCallback (internal/process/next_tick.js:61:11)

@lpinca
Copy link
Member

lpinca commented Feb 2, 2020

I don't understand this PR. It doesn't use Node.js primordials so what is this protecting against?

@falsandtru
Copy link
Author

Global variable reference greatly degrades the runtime performance as follows.

[BAD 0.7%]
Global variable reference
x 3,803,038 ops/sec ±1.03% (57 runs sampled)

[BAD 0%]
Global variable reference x10
x 331,805 ops/sec ±20.80% (52 runs sampled)

[BAD 0.5%]
Instantiation of global variable
x 2,894,146 ops/sec ±2.00% (57 runs sampled)

[Fastest]
Local variable reference
x 527,495,329 ops/sec ±0.58% (59 runs sampled)

Instantiation of local variable
x 473,611,915 ops/sec ±20.21% (52 runs sampled)

https://falsandtru.github.io/benchmark/suites/10/

This PR prevents the performance degradation.

@lpinca
Copy link
Member

lpinca commented Feb 2, 2020

Can you show the difference by running eventemitter3 benchmarks before and after the change?

@lpinca
Copy link
Member

lpinca commented Feb 2, 2020

The use of primordials caused performance regressions on Nodes.js not boost nodejs/node#29766.

@lpinca
Copy link
Member

lpinca commented Feb 2, 2020

$ node -v
v13.7.0
$ cat index.js
'use strict';

const Benchmark = require('benchmark');

const suite = new Benchmark.Suite();
const Arr = Array;

let ret;

suite.add('local', function() {
  ret = new Arr();
});

suite.add('global', function() {
  ret = new Array();
});

suite.on('cycle', function(event) {
  console.log(event.target.toString());
});

suite.on('complete', function() {
  console.log('Fastest is ' + this.filter('fastest').map('name'));
});

suite.run({ async: true });
$ node index.js
local x 75,396,008 ops/sec ±0.35% (94 runs sampled)
global x 76,764,603 ops/sec ±0.10% (98 runs sampled)
Fastest is global

@falsandtru
Copy link
Author

That issue lists two causes but this PR doesn't use either.

The regressions seem to come from two types of code patterns:

  1. Calling functions through Function.prototype.{call, apply} instead of just calling them directly. There is an issue opened in the upstream by @\bmeck
    https://bugs.chromium.org/p/v8/issues/detail?id=9702
  2. Looking up properties from fronzen objects - objects from our primordials namespace are frozen so lookups like const { Reflect } = primordials; Reflect.apply(...); is slower than just Reflect.apply when Reflect comes from the global object. This can be mitigated by caching the lookup results upfront, e.g. #29633 by @\mcollina There is also a fairly odd tracking issue for this in the upstream: https://bugs.chromium.org/p/v8/issues/detail?id=6831

@lpinca
Copy link
Member

lpinca commented Feb 2, 2020

Yes, the point is that the use of primordials did not cause any performance improvement in Node.js

@falsandtru
Copy link
Author

Confirmed. Looks like it is difficult to improve the performance on browsers without decreasing the performance 1-2% on Node.js.

@kmashint
Copy link

kmashint commented Sep 6, 2024

Note Function('return this')() breaks a typical Content-Security-Policy, so it shouldn't resort to that.

@cyfung1031
Copy link

You can use const $Object = Object, $Array = Array to do the same, but no one cares about such micro optimization except you.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants