Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Avoid global variable reference #212

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

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.

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.

3 participants