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.

Sign up for GitHub

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

Jump to bottom

lib: refactor prototype and Function.call.bind usage #18773

Closed
wants to merge 1 commit into from
Closed

lib: refactor prototype and Function.call.bind usage #18773

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions lib/.eslintrc.yaml
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,3 +5,7 @@ rules:
node-core/no-let-in-for-declaration: error
node-core/lowercase-name-for-primitive: error
node-core/non-ascii-character: error

# Possible Errors
# http://eslint.org/docs/rules/#possible-errors
no-prototype-builtins: error
3 changes: 2 additions & 1 deletion lib/assert.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const { parseExpressionAt } = require('internal/deps/acorn/dist/acorn');
const { inspect } = require('util');
const { EOL } = require('os');
const nativeModule = require('native_module');
const { ObjectIsPrototypeOf } = require('primordials');

// Escape control characters but not \n and \t to keep the line breaks and
// indentation intact.
Expand Down Expand Up @@ -400,7 +401,7 @@ function expectedException(actual, expected, msg) {
if (expected.prototype !== undefined && actual instanceof expected) {
return true;
}
if (Error.isPrototypeOf(expected)) {
if (ObjectIsPrototypeOf(Error, expected)) {
return false;
}
return expected.call({}, actual) === true;
Expand Down
4 changes: 3 additions & 1 deletion lib/internal/async_hooks.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
'use strict';

const { ObjectHasOwnProperty } = require('primordials');
const errors = require('internal/errors');
const async_wrap = process.binding('async_wrap');

/* async_hook_fields is a Uint32Array wrapping the uint32_t array of
* Environment::AsyncHooks::fields_[]. Each index tracks the number of active
* hooks for each type.
Expand Down Expand Up @@ -252,7 +254,7 @@ function newAsyncId() {
}

function getOrSetAsyncId(object) {
if (object.hasOwnProperty(async_id_symbol)) {
if (ObjectHasOwnProperty(object, async_id_symbol)) {
return object[async_id_symbol];
}

Expand Down
43 changes: 40 additions & 3 deletions lib/internal/bootstrap_node.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
(function(process) {
let internalBinding;
const exceptionHandlerState = { captureFn: null };
const primordials = makePrimordials();

function startup() {
const EventEmitter = NativeModule.require('events');
Expand Down Expand Up @@ -411,7 +412,7 @@
addCommandLineAPI('require', makeRequireFunction(consoleAPIModule));
const config = {};
for (const key of Object.keys(wrappedConsole)) {
if (!originalConsole.hasOwnProperty(key))
if (!primordials.ObjectHasOwnProperty(originalConsole, key))
continue;
// If global console has the same method as inspector console,
// then wrap these two methods into one. Native wrapper will preserve
Expand All @@ -422,7 +423,7 @@
config);
}
for (const key of Object.keys(originalConsole)) {
if (wrappedConsole.hasOwnProperty(key))
if (primordials.ObjectHasOwnProperty(wrappedConsole, key))
continue;
wrappedConsole[key] = originalConsole[key];
}
Expand Down Expand Up @@ -601,6 +602,8 @@
NativeModule.require = function(id) {
if (id === 'native_module') {
return NativeModule;
} else if (id === 'primordials') {
return primordials;
}

const cached = NativeModule.getCached(id);
Expand Down Expand Up @@ -643,7 +646,7 @@
};

NativeModule.exists = function(id) {
return NativeModule._source.hasOwnProperty(id);
return primordials.ObjectHasOwnProperty(NativeModule._source, id);
};

if (config.exposeInternals) {
Expand Down Expand Up @@ -704,4 +707,38 @@
};

startup();

function makePrimordials() {
const ReflectApply = Reflect.apply;

// This function is borrowed from the function with the same name on V8
// Extras' `utils` object. V8 implements Reflect.apply very efficiently in
// conjunction with the spread syntax, such that no additional special case
// is needed for function calls w/o arguments.
// Refs: https://git.io/vAOyO
function uncurryThis(func) {
return (thisArg, ...args) => ReflectApply(func, thisArg, args);
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@hashseed @bmeurer @devsnek can you confirm that this has zero overhead? Some of these calls sits in hot paths and I do not want to add an utility that we need to be careful in using.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I see a 17% performance regression with this benchmark on Node 8, but with newest V8 there is none.

const ReflectApply = Reflect.apply;

function uncurryThis(func) {
  return (thisArg, ...args) => ReflectApply(func, thisArg, args);
}
var StringStartsWith = uncurryThis(String.prototype.startsWith);

var s = "Hi Matteo";
function test1() {
  var r = 0;
  console.time("monkey-patchable");
  for (var i = 0; i < 1E7; i++) {
    if (s.startsWith(i)) r++;
  }
  console.timeEnd("monkey-patchable");
  return r;
}

function test2() {
  var r = 0;
  console.time("cached original");
  for (var i = 0; i < 1E7; i++) {
    if (StringStartsWith(s, i)) r++;
  }
  console.timeEnd("cached original");
  return r;
}

test1();
test2();

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

wow! Good to know!
Which version of V8 did you check? what we have on master? What we plan to have for Node 10?

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also ran with current Node master. There is no regression there either.


return {
uncurryThis,

ArrayJoin: uncurryThis(Array.prototype.join),
ArrayMap: uncurryThis(Array.prototype.map),

FunctionBind: uncurryThis(Function.prototype.bind),

JSONParse: JSON.parse,

ObjectIsPrototypeOf: uncurryThis(Object.prototype.isPrototypeOf),
ObjectHasOwnProperty: uncurryThis(Object.prototype.hasOwnProperty),
ObjectKeys: Object.keys,

ReflectApply,
ReflectHas: Reflect.has,

StringReplace: uncurryThis(String.prototype.replace),
StringStartsWith: uncurryThis(String.prototype.startsWith),
};
}
});
9 changes: 5 additions & 4 deletions lib/internal/crypto/sig.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,7 @@ const {
const { isArrayBufferView } = require('internal/util/types');
const { Writable } = require('stream');
const { inherits } = require('util');
const { ObjectHasOwnProperty } = require('primordials');

function Sign(algorithm, options) {
if (!(this instanceof Sign))
Expand Down Expand Up @@ -55,7 +56,7 @@ Sign.prototype.sign = function sign(options, encoding) {

// Options specific to RSA
var rsaPadding = RSA_PKCS1_PADDING;
if (options.hasOwnProperty('padding')) {
if (ObjectHasOwnProperty(options, 'padding')) {
if (options.padding === options.padding >> 0) {
rsaPadding = options.padding;
} else {
Expand All @@ -66,7 +67,7 @@ Sign.prototype.sign = function sign(options, encoding) {
}

var pssSaltLength = RSA_PSS_SALTLEN_AUTO;
if (options.hasOwnProperty('saltLength')) {
if (ObjectHasOwnProperty(options, 'saltLength')) {
if (options.saltLength === options.saltLength >> 0) {
pssSaltLength = options.saltLength;
} else {
Expand Down Expand Up @@ -114,7 +115,7 @@ Verify.prototype.verify = function verify(options, signature, sigEncoding) {

// Options specific to RSA
var rsaPadding = RSA_PKCS1_PADDING;
if (options.hasOwnProperty('padding')) {
if (ObjectHasOwnProperty(options, 'padding')) {
if (options.padding === options.padding >> 0) {
rsaPadding = options.padding;
} else {
Expand All @@ -125,7 +126,7 @@ Verify.prototype.verify = function verify(options, signature, sigEncoding) {
}

var pssSaltLength = RSA_PSS_SALTLEN_AUTO;
if (options.hasOwnProperty('saltLength')) {
if (ObjectHasOwnProperty(options, 'saltLength')) {
if (options.saltLength === options.saltLength >> 0) {
pssSaltLength = options.saltLength;
} else {
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/loader/CreateDynamicModule.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -2,8 +2,7 @@

const { ModuleWrap } = internalBinding('module_wrap');
const debug = require('util').debuglog('esm');
const ArrayJoin = Function.call.bind(Array.prototype.join);
const ArrayMap = Function.call.bind(Array.prototype.map);
const { ArrayJoin, ArrayMap } = require('primordials');

const createDynamicModule = (exports, url = '', evaluate) => {
debug(
Expand Down
2 changes: 1 addition & 1 deletion lib/internal/loader/DefaultResolve.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,7 +9,7 @@ const { realpathSync } = require('fs');
const preserveSymlinks = !!process.binding('config').preserveSymlinks;
const errors = require('internal/errors');
const { resolve: moduleWrapResolve } = internalBinding('module_wrap');
const StringStartsWith = Function.call.bind(String.prototype.startsWith);
const { StringStartsWith } = require('primordials');
const { getURLFromFilePath, getPathFromURL } = require('internal/url');

const realpathCache = new Map();
Expand Down
3 changes: 1 addition & 2 deletions lib/internal/loader/Loader.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,8 +6,7 @@ const ModuleJob = require('internal/loader/ModuleJob');
const defaultResolve = require('internal/loader/DefaultResolve');
const createDynamicModule = require('internal/loader/CreateDynamicModule');
const translators = require('internal/loader/Translators');

const FunctionBind = Function.call.bind(Function.prototype.bind);
const { FunctionBind } = require('primordials');

const debug = require('util').debuglog('esm');

Expand Down
5 changes: 2 additions & 3 deletions lib/internal/loader/Translators.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,8 +13,7 @@ const { URL } = require('url');
const debug = require('util').debuglog('esm');
const readFileAsync = require('util').promisify(fs.readFile);
const readFileSync = fs.readFileSync;
const StringReplace = Function.call.bind(String.prototype.replace);
const JsonParse = JSON.parse;
const { StringReplace, JSONParse } = require('primordials');

const translators = new SafeMap();
module.exports = translators;
Expand Down Expand Up @@ -82,7 +81,7 @@ translators.set('json', async (url) => {
const pathname = internalURLModule.getPathFromURL(new URL(url));
const content = readFileSync(pathname, 'utf8');
try {
const exports = JsonParse(internalCJSModule.stripBOM(content));
const exports = JSONParse(internalCJSModule.stripBOM(content));
reflect.exports.default.set(exports);
} catch (err) {
err.message = pathname + ': ' + err.message;
Expand Down
11 changes: 1 addition & 10 deletions lib/internal/util/types.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,15 +1,6 @@
'use strict';

const ReflectApply = Reflect.apply;

// This function is borrowed from the function with the same name on V8 Extras'
// `utils` object. V8 implements Reflect.apply very efficiently in conjunction
// with the spread syntax, such that no additional special case is needed for
// function calls w/o arguments.
// Refs: https://github.com/v8/v8/blob/d6ead37d265d7215cf9c5f768f279e21bd170212/src/js/prologue.js#L152-L156
function uncurryThis(func) {
return (thisArg, ...args) => ReflectApply(func, thisArg, args);
}
const { uncurryThis } = require('primordials');

const TypedArrayPrototype = Object.getPrototypeOf(Uint8Array.prototype);

Expand Down