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

performance: fix timerify bug #40625

Closed
wants to merge 4 commits into from
Closed

performance: fix timerify bug #40625

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
36c886f
performance: fix timerify bug
iam-frankqiu Oct 27, 2021
304d5df
perf_hooks: add a test and drop isConstructor
iam-frankqiu Oct 27, 2021
47edde3
perf_hooks: alter the test and format timerify
iam-frankqiu Oct 28, 2021
591f94b
doc: add a comment to the fn
iam-frankqiu Oct 30, 2021
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
8 changes: 2 additions & 6 deletions lib/internal/perf/timerify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,10 +21,6 @@ const {
isHistogram
} = require('internal/histogram');

const {
isConstructor,
} = internalBinding('util');

const {
codes: {
ERR_INVALID_ARG_TYPE,
Expand Down Expand Up @@ -73,11 +69,11 @@ function timerify(fn, options = {}) {

if (fn[kTimerified]) return fn[kTimerified];

const constructor = isConstructor(fn);
const isClass = (fn) => /^\s*class/.test(fn.toString());
Copy link
Member

Choose a reason for hiding this comment

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

This would fail if someone is using "old" classes which can be common when transpiling cross-platform code that targets old targets (for example: using tooling that uses es5 at any point)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

This would fail if someone is using "old" classes which can be common when transpiling cross-platform code that targets old targets (for example: using tooling that uses es5 at any point)

Maybe you can add a test on test-performance-function.js or just show me an example function(I will add to the former file). I will solve the problem to ensure all tests have been passed.

Copy link
Member

Choose a reason for hiding this comment

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

Sure

function MyClass {
  this.foo = 15;
}
MyClass.prototype.sayFoo {
  console.log(this.foo);
}
const instance = new MyClass(); ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

this.foo = 15;
Hasn't it a return value ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sure

function MyClass {
  this.foo = 15;
}
MyClass.prototype.sayFoo {
  console.log(this.foo);
}
const instance = new MyClass(); ?

If a function is like this. Actually, we can't judge which way is to call. Because using new to call and invoke directly is different. So I think we just invoke it directly if the first augment is a function. Using new to call if it is a class. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

I think that will not work for a large chunk of our ecosystem unfortunately and there are people who still use "old style" constructors or have existing projects that do it. So I think for me personally this not breaking for "old style" classes is important.


function timerified(...args) {
const start = now();
const result = constructor ?
const result = isClass(fn) ?
Copy link
Member

Choose a reason for hiding this comment

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

Maybe we should change the check from "is it a constructor?" to "is it called as a constructor?". i.e. new.target !== undefined. wdyt?

Copy link
Member

Choose a reason for hiding this comment

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

That sounds like a very reasonable approach to me

Copy link
Member

Choose a reason for hiding this comment

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

Note new.target can be set but the return value can still be overridden by returning something different - so a constructor check is would still need to check the return value

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe we should change the check from "is it a constructor?" to "is it called as a constructor?". i.e. new.target !== undefined. wdyt?

Actually. I have considered this problem. It seems like new.target only can be used within a function. I didn't find any way to use it outside of a function.

Copy link
Member

Choose a reason for hiding this comment

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

It seems like new.target only can be used within a function. I didn't find any way to use it outside of a function.

This should be fine. timerified is a function.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like new.target only can be used within a function. I didn't find any way to use it outside of a function.

This should be fine. timerified is a function.

I think we don't need to judge if it is called a constructor. we only judge whether the argument is a function or not. if is a class we use new to call, else we invoked directly.

Copy link
Member

Choose a reason for hiding this comment

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

I don't agree. In my opinion, timerified should be a proxy to the function and behave as much as possible the same as if the function was called directly.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I don't agree. In my opinion, timerified should be a proxy to the function and behave as much as possible the same as if the function was called directly.

There must have been some misunderstanding in there. What I do is just like you said. Are there any conflicts?

Copy link
Contributor

Choose a reason for hiding this comment

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

The conflict is that, in JavaScript, a function can be a class. What Michaël is saying is that, no matter what we can infer of the argument being a "class" or not, if timerified is invoke using new keyboard (i.e. if new.target !== undefined), we should pass that along.

ReflectConstruct(fn, args, fn) :
ReflectApply(fn, this, args);
if (!constructor && typeof result?.finally === 'function') {
iam-frankqiu marked this conversation as resolved.
Show resolved Hide resolved
Expand Down
12 changes: 12 additions & 0 deletions test/parallel/test-performance-function.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -13,6 +13,18 @@ const {
setTimeout: sleep
} = require('timers/promises');

{
const perf = performance.timerify(function foo() {
let sum = 0;
for (let i = 0; i < 101; i++) {
sum += i;
}
return sum;
});
const result = perf();
assert.strictEqual(result, 5050);
}

{
// Intentional non-op. Do not wrap in common.mustCall();
const n = performance.timerify(function noop() {});
Expand Down