Skip to content

Avoid global variable reference - #212

Open
falsandtru wants to merge 1 commit into
primus:masterfrom
falsandtru:globals
Open

Avoid global variable reference#212
falsandtru wants to merge 1 commit into
primus:masterfrom
falsandtru:globals

Conversation

@falsandtru

Copy link
Copy Markdown

@falsandtru

Copy link
Copy Markdown
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

lpinca commented Feb 2, 2020

Copy link
Copy Markdown
Member

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

@falsandtru

Copy link
Copy Markdown
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

lpinca commented Feb 2, 2020

Copy link
Copy Markdown
Member

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

@lpinca

lpinca commented Feb 2, 2020

Copy link
Copy Markdown
Member

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

@lpinca

lpinca commented Feb 2, 2020

Copy link
Copy Markdown
Member
$ 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
Copy Markdown
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

lpinca commented Feb 2, 2020

Copy link
Copy Markdown
Member

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

@falsandtru

Copy link
Copy Markdown
Author

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

@kmashint

kmashint commented Sep 6, 2024

Copy link
Copy Markdown

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

@cyfung1031

Copy link
Copy Markdown

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