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

perf_hooks: fix timerify bug #42883

Open
wants to merge 4 commits into
base: main
Choose a base branch
from
Open

perf_hooks: fix timerify bug #42883

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
704c7ed
perf_hooks: fix timerify bug
iam-frankqiu Apr 27, 2022
da9e49a
fixup! return result in processComplete
himself65 Apr 28, 2022
4b62b68
fixup! rewrite logic
himself65 May 1, 2022
3a28c1b
fixup! add test case and lint fix
himself65 May 2, 2022
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
18 changes: 8 additions & 10 deletions lib/internal/perf/timerify.js
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,7 +1,6 @@
'use strict';

const {
FunctionPrototypeBind,
ObjectDefineProperties,
MathCeil,
ReflectApply,
Expand Down Expand Up @@ -75,18 +74,17 @@ function timerify(fn, options = {}) {
const result = constructor ?
ReflectConstruct(fn, args, fn) :
ReflectApply(fn, this, args);
if (!constructor && typeof result?.finally === 'function') {
return result.finally(
FunctionPrototypeBind(
processComplete,
result,
fn.name,
start,
args,
histogram));
if (!constructor && typeof result?.then === 'function') {
return result.then(
Copy link
Contributor

Choose a reason for hiding this comment

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

Here we retrieve result.then twice which theoretically not work like JS spec strictly. We'd better write a helper to ensure such subtle semantic. (Not sure whether Node.js already have such internal thenable helpers).

Copy link
Member

Choose a reason for hiding this comment

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

What do you mean?

Copy link
Member

Choose a reason for hiding this comment

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

Do you mean like this?

node/lib/assert.js

Lines 763 to 771 in 1e76165

function checkIsPromise(obj) {
// Accept native ES6 promises and promises that are implemented in a similar
// way. Do not accept thenables that use a function as `obj` and that have no
// `catch` handler.
return isPromise(obj) ||
(obj !== null && typeof obj === 'object' &&
typeof obj.then === 'function' &&
typeof obj.catch === 'function');
}

Copy link
Contributor

Choose a reason for hiding this comment

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

I mean, if follow JS spec strictly, the code should be:

const {then} = result
if (typeof then === 'function') {
  ReflectApply(then, result, [callback])
}

Because theoretically result.then could be a getter, and two retrieve could give u different result.

Copy link
Contributor

Choose a reason for hiding this comment

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

I also don't understand why checkIsPromise "do not accept thenables that use a function as obj and that have no catch handler" intentionally, this limitation seems wrong to me.

function wrappedTimerifiedPromise(value) {
processComplete(fn.name, start, args, histogram);
Copy link
Contributor

Choose a reason for hiding this comment

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

One idea which is not relate to the bug directly, but:

currently, now is called in the processComplete (duration = now() - start) , but consider the goal is to calc the time as precise as possible, it may be better to calc it outside like:

const t = now()
processComplete(fn.name, start - t, args, histogram)

(also need to modify other related places, omit here)

This could rule out the invoking time of processComplete, in the cases of microbenchmark, such cost can't be ignored.

return value;
}
);
}
processComplete(fn.name, start, args, histogram);
return result;

}

ObjectDefineProperties(timerified, {
Expand Down
14 changes: 14 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 @@ -89,6 +89,20 @@ const {
assert.strictEqual(p.name, 'timerified timerified m');
}

// Function
{
const f1 = () => 1;
const f2 = async () => 2;
const h1 = createHistogram();
const h2 = createHistogram();
const g1 = performance.timerify(f1, { histogram: h1 });
const g2 = performance.timerify(f2, { histogram: h2 });
g1();
g2().then(common.mustCall(() => {
assert.strictEqual(h1.count, h2.count);
}));
}

(async () => {
const histogram = createHistogram();
const m = (a, b = 1) => {};
Expand Down