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

url: use util's getConstructorOf #12526

Closed
wants to merge 2 commits into from
Closed

url: use util's getConstructorOf #12526

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
a5dad81
util: move getConstructorOf() to internal
TimothyGu Apr 20, 2017
0f495ff
url: use internal/util's getConstructorOf
TimothyGu Apr 20, 2017
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
14 changes: 2 additions & 12 deletions lib/internal/url.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@ const {
hexTable,
isHexTable
} = require('internal/querystring');
const { getConstructorOf } = require('internal/util');
const binding = process.binding('url');
const context = Symbol('context');
const cannotBeBase = Symbol('cannot-be-base');
Expand Down Expand Up @@ -184,17 +185,6 @@ function onParseHashComplete(flags, protocol, username, password,
}
}

function getEligibleConstructor(obj) {
while (obj !== null) {
if (Object.prototype.hasOwnProperty.call(obj, 'constructor') &&
typeof obj.constructor === 'function') {
return obj.constructor;
}
obj = Object.getPrototypeOf(obj);
}
return null;
}

class URL {
constructor(input, base) {
// toUSVString is not needed.
Expand Down Expand Up @@ -225,7 +215,7 @@ class URL {
if (typeof depth === 'number' && depth < 0)
return opts.stylize('[Object]', 'special');

const ctor = getEligibleConstructor(this);
const ctor = getConstructorOf(this);

const obj = Object.create({
constructor: ctor === null ? URL : ctor
Expand Down
15 changes: 15 additions & 0 deletions lib/internal/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -187,3 +187,18 @@ exports.convertToValidSignal = function convertToValidSignal(signal) {

throw new Error('Unknown signal: ' + signal);
};

exports.getConstructorOf = function getConstructorOf(obj) {
while (obj) {
var descriptor = Object.getOwnPropertyDescriptor(obj, 'constructor');
Copy link

Choose a reason for hiding this comment

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

The performance of Object.getOwnPropertyDescriptor is about 17 times slower than Object.prototype.hasOwnProperty.call. Perhaps the code from getEligibleConstructor can be used instead.

Copy link
Member Author

Choose a reason for hiding this comment

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

That would be a breaking change for a class that defines constructor as accessors.

Copy link

Choose a reason for hiding this comment

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

I've been asking myself in which case I'll need to do that. And I don't find the answer. Can you provide me some example of a case where you really want to do that?

Copy link

Choose a reason for hiding this comment

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

I'll answer myself: even if I don't found any case, it should be a breaking change if someone does it that way.
But remember that URL constructor always has been using hasOwnProperty and now it will use getOwnPropetyDescriptor, so it means that a -1700% performance will happen. I'm ok with that but perhaps someone is not.

Copy link
Member

Choose a reason for hiding this comment

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

@TimothyGu the code currently explicitly skips over any get constructor() {} - previously in url, it only used hasOwnProperty, so as-is, this is a breaking change for URL. it's only in lib/util that this is keeping the behavior the same.

In other words, any deduplication of this code seems to be a breaking change in one place or the other.

Copy link
Member Author

Choose a reason for hiding this comment

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

@ljharb, the URL change that added hasOwnProperty has not been released yet. In fact, the WHATWG URL API is still in experimental phase, and so compatibility on the URL side is not an issue.

@jseijas the URL constructor is not using this function, only the custom inspection function (which is not performance-sensitive at all) is using it.

if (descriptor !== undefined &&
typeof descriptor.value === 'function' &&
descriptor.value.name !== '') {
return descriptor.value;
}

obj = Object.getPrototypeOf(obj);
}

return null;
};
18 changes: 1 addition & 17 deletions lib/util.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -278,22 +278,6 @@ function arrayToHash(array) {
}


function getConstructorOf(obj) {
while (obj) {
var descriptor = Object.getOwnPropertyDescriptor(obj, 'constructor');
if (descriptor !== undefined &&
typeof descriptor.value === 'function' &&
descriptor.value.name !== '') {
return descriptor.value;
}

obj = Object.getPrototypeOf(obj);
}

return null;
}


function ensureDebugIsInitialized() {
if (Debug === undefined) {
const runInDebugContext = require('vm').runInDebugContext;
Expand Down Expand Up @@ -410,7 +394,7 @@ function formatValue(ctx, value, recurseTimes) {
});
}

var constructor = getConstructorOf(value);
var constructor = internalUtil.getConstructorOf(value);
Copy link
Contributor

@mscdex mscdex Apr 20, 2017 •
edited
Loading

Choose a reason for hiding this comment

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

Can we extract this at the top of the file so we're not referencing internalUtil each time? Until lib/internal/util.js gets refactored to use the "new" module.exports = { ... } format, the property lookup could be slow.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have done just that in 9077b48 / #11404.

Copy link
Member

Choose a reason for hiding this comment

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

Btw, internal/util has now been refactored to use the module.exports = { ... } format.


// Some type of object without properties can be shortcutted.
if (keys.length === 0) {
Expand Down