Skip to content

Commit

Permalink
errors: significantly improve NodeError creation performance
Browse files Browse the repository at this point in the history 
The improvement comes from the following changes:

1. Less properties are defined on the error instances, especially
   no Symbol.
2. The check if something is a NodeError or not is not needed
   anymore.
3. The setup of NodeError now makes sure that all heavy lifting is
   done up front instead of doing that during instance creation.

To align with the WPT tests, the NodeErrors must have their
constructor set to their base class. This was not the case for
SystemErrors and is now aligned.

Signed-off-by: Ruben Bridgewater <ruben@bridgewater.de>
  • Loading branch information
@BridgeAR
BridgeAR committed Feb 14, 2023
1 parent b51fb36 commit 15854a8
Show file tree
Hide file tree
Showing 9 changed files with 110 additions and 108 deletions.
8 changes: 3 additions & 5 deletions lib/internal/assert/assertion_error.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -426,13 +426,15 @@ class AssertionError extends Error {
setStackTraceLimit(stackTraceLimit);

this.generatedMessage = !message;

ObjectDefineProperty(this, 'name', {
__proto__: null,
value: 'AssertionError [ERR_ASSERTION]',
value: 'AssertionError',
enumerable: false,
writable: true,
configurable: true
});

this.code = 'ERR_ASSERTION';
if (details) {
this.actual = undefined;
Expand All @@ -451,10 +453,6 @@ class AssertionError extends Error {
this.operator = operator;
}
ErrorCaptureStackTrace(this, stackStartFn || stackStartFunction);
// Create error message including the error code in the name.
this.stack; // eslint-disable-line no-unused-expressions
// Reset the name.
this.name = 'AssertionError';
}

toString() {
Expand Down
5 changes: 4 additions & 1 deletion lib/internal/console/constructor.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -421,7 +421,10 @@ const consoleMethods = {
trace: function trace(...args) {
const err = {
name: 'Trace',
message: this[kFormatForStderr](args)
message: this[kFormatForStderr](args),
toString() {
return `${this.name}: ${this.message}`;
}
};
ErrorCaptureStackTrace(err, trace);
this.error(err.stack);
Expand Down
139 changes: 72 additions & 67 deletions lib/internal/errors.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -31,6 +31,7 @@ const {
ObjectDefineProperty,
ObjectDefineProperties,
ObjectKeys,
ObjectPrototype,
RangeError,
ReflectApply,
RegExpPrototypeExec,
Expand All @@ -50,8 +51,6 @@ const {
URIError,
} = primordials;

const kIsNodeError = Symbol('kIsNodeError');

const isWindows = process.platform === 'win32';

const messages = new SafeMap();
Expand All @@ -77,6 +76,8 @@ const MainContextError = Error;
const overrideStackTrace = new SafeWeakMap();
const kNoOverride = Symbol('kNoOverride');
const nodeInternalPrefix = '__node_internal_';
const MainContextObjectToString = ObjectPrototype.toString;

const prepareStackTrace = (globalThis, error, trace) => {
// API for node internals to override error stack formatting
// without interfering with userland code.
Expand Down Expand Up @@ -111,8 +112,13 @@ const prepareStackTrace = (globalThis, error, trace) => {
// at function (file)
// at file
let errorString;
if (kIsNodeError in error) {
errorString = `${error.name} [${error.code}]: ${error.message}`;
// Do not use primordials here: we intercept user code here.
if (typeof error.toString === 'function' &&
error.toString !== MainContextObjectToString) {
errorString = error.toString();
if (errorString === '[Object object]') {
errorString = ErrorPrototypeToString(error);
}
} else {
errorString = ErrorPrototypeToString(error);
}
Expand Down Expand Up @@ -220,8 +226,9 @@ function inspectWithNoCustomRetry(obj, options) {
// and may have .path and .dest.
class SystemError extends Error {
constructor(key, context) {
super();
const prefix = getMessage(key, [], this);
const msg = messages.get(key);

const prefix = getMessage(key, msg, []);
let message = `${prefix}: ${context.syscall} returned ` +
`${context.code} (${context.message})`;

Expand All @@ -230,30 +237,18 @@ class SystemError extends Error {
if (context.dest !== undefined)
message += ` => ${context.dest}`;

super(message);

this.code = key;

ObjectDefineProperties(this, {
[kIsNodeError]: {
__proto__: null,
value: true,
enumerable: false,
writable: false,
configurable: true,
},
name: {
__proto__: null,
value: 'SystemError',
enumerable: false,
writable: true,
configurable: true,
},
message: {
__proto__: null,
value: message,
enumerable: false,
writable: true,
configurable: true,
},
info: {
__proto__: null,
value: context,
Expand Down Expand Up @@ -337,45 +332,53 @@ class SystemError extends Error {
}

function makeSystemErrorWithCode(key) {
return class NodeError extends SystemError {
const clazz = class NodeError extends SystemError {
constructor(ctx) {
super(key, ctx);
}
};
// The constructor must be the Base class to align with the WPT tests.
SystemError.prototype.constructor = Error;
return clazz;
}

function makeNodeErrorWithCode(Base, key) {
return function NodeError(...args) {
const error = new Base();
const message = getMessage(key, args, error);
ObjectDefineProperties(error, {
[kIsNodeError]: {
__proto__: null,
value: true,
enumerable: false,
writable: false,
configurable: true,
},
message: {
__proto__: null,
value: message,
enumerable: false,
writable: true,
configurable: true,
},
toString: {
__proto__: null,
value() {
return `${this.name} [${key}]: ${this.message}`;
},
enumerable: false,
writable: true,
configurable: true,
},
});
error.code = key;
return error;
};
function makeNodeErrorWithCode(Base, key, val) {
let clazz;
if (typeof val === 'string') {
clazz = class NodeError extends Base {
constructor(...args) {
const message = getMessage(key, val, args);
super(message);
this.code = key;
}

toString() {
return `${this.name} [${key}]: ${this.message}`;
}
};
} else {
clazz = class NodeError extends Base {
constructor(...args) {
super();
const message = getFnMessage(key, val, args, this);
ObjectDefineProperty(this, 'message', {
__proto__: null,
value: message,
enumerable: false,
writable: true,
configurable: true,
});
this.code = key;
}

toString() {
return `${this.name} [${key}]: ${this.message}`;
}
};
}
// The constructor must be the Base class to align with the WPT tests.
clazz.prototype.constructor = Base;
return clazz;
}

/**
Expand Down Expand Up @@ -409,32 +412,34 @@ function E(sym, val, def, ...otherClasses) {
if (def === SystemError) {
def = makeSystemErrorWithCode(sym);
} else {
def = makeNodeErrorWithCode(def, sym);
def = makeNodeErrorWithCode(def, sym, val);
}

if (otherClasses.length !== 0) {
otherClasses.forEach((clazz) => {
def[clazz.name] = makeNodeErrorWithCode(clazz, sym);
def[clazz.name] = makeNodeErrorWithCode(clazz, sym, val);
});
}
codes[sym] = def;
}

function getMessage(key, args, self) {
const msg = messages.get(key);

function getFnMessage(key, fn, args, self) {
assert ??= require('internal/assert');

const expectedLength = messageArguments.get(key);

if (typeof msg === 'function') {
assert(
expectedLength <= args.length,
`Code: ${key}; The provided arguments length (${args.length}) does not ` +
`match the required ones (${expectedLength}).`
);
return ReflectApply(msg, self, args);
}
assert(
expectedLength <= args.length,
`Code: ${key}; The provided arguments length (${args.length}) does not ` +
`match the required ones (${expectedLength}).`
);
return ReflectApply(fn, self, args);
}

function getMessage(key, msg, args) {
assert ??= require('internal/assert');

const expectedLength = messageArguments.get(key);

assert(
expectedLength === args.length,
Expand Down Expand Up @@ -858,14 +863,14 @@ module.exports = {
fatalExceptionStackEnhancers,
formatList,
genericNodeError,
messages,
getMessage,
hideInternalStackFrames,
hideStackFrames,
inspectWithNoCustomRetry,
setStackTraceLimit,
isStackOverflowError,
kEnhanceStackBeforeInspector,
kIsNodeError,
kNoOverride,
maybeOverridePrepareStackTrace,
overrideStackTrace,
Expand Down
13 changes: 3 additions & 10 deletions lib/internal/http2/util.js
View file
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,6 @@ const {
Error,
MathMax,
Number,
ObjectDefineProperty,
ObjectKeys,
SafeSet,
String,
Expand All @@ -28,9 +27,9 @@ const {
ERR_INVALID_ARG_TYPE,
ERR_INVALID_HTTP_TOKEN
},
messages,
getMessage,
hideStackFrames,
kIsNodeError,
} = require('internal/errors');

const kSensitiveHeaders = Symbol('nodejs.http2.sensitiveHeaders');
Expand Down Expand Up @@ -546,18 +545,12 @@ function mapToHeaders(map,

class NghttpError extends Error {
constructor(integerCode, customErrorCode) {
const msg = messages.get(customErrorCode);
super(customErrorCode ?
getMessage(customErrorCode, [], null) :
getMessage(customErrorCode, msg, []) :
binding.nghttp2ErrorString(integerCode));
this.code = customErrorCode || 'ERR_HTTP2_ERROR';
this.errno = integerCode;
ObjectDefineProperty(this, kIsNodeError, {
__proto__: null,
value: true,
enumerable: false,
writable: false,
configurable: true,
});
}

toString() {
Expand Down
Loading

0 comments on commit 15854a8

Please sign in to comment.