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.

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

feat(node, cloudflare): fill in api surface for node:perf_hooks #257

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
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
1 change: 1 addition & 0 deletions src/presets/cloudflare.ts
Original file line number Diff line number Diff line change
Expand Up @@ -25,6 +25,7 @@ const hybridNodeCompatModules = [
"async_hooks",
"buffer",
"crypto",
"perf_hooks",
"process",
"util",
"util/types",
Expand Down
12 changes: 11 additions & 1 deletion src/presets/nodeless.ts
Original file line number Diff line number Diff line change
Expand Up @@ -89,7 +89,17 @@ const nodeless: Preset & { alias: Map<string, string> } = {
global: "unenv/runtime/node/_global",
process: "unenv/runtime/node/process",
Buffer: ["buffer", "Buffer"],
performance: "unenv/runtime/polyfill/performance",
performance: ["perf_hooks", "performance"],
Performance: ["perf_hooks", "Performance"],
PerformanceEntry: ["perf_hooks", "PerformanceEntry"],
PerformanceMark: ["perf_hooks", "PerformanceMark"],
PerformanceMeasure: ["perf_hooks", "PerformanceMeasure"],
PerformanceObserver: ["perf_hooks", "PerformanceObserver"],
PerformanceObserverEntryList: [
"perf_hooks",
"PerformanceObserverEntryList",
],
PerformanceResourceTiming: ["perf_hooks", "PerformanceResourceTiming"],
Copy link
Member

@pi0 pi0 Jun 18, 2024

Choose a reason for hiding this comment

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

To be honest, I'm still not sure about this part. Initial injections in nodeless (global, process, and Buffer) are all Node.js specific global which were safe to inject globally. Performance API is a Web standard on the other hand. By adding this injections we are effectively overriding any native global implementation in user-code to unenv/node.

I think this should be an explicit opt-in. wrangler preset (#262) is a good candidate because i suppose you also require explicit (Node-compat) from users to enable this and then things make sense.

Copy link
Member

Choose a reason for hiding this comment

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

(i would suggest to revert this part and split to another PR to unblock)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Performance API is a Web standard on the other hand. By adding this injections we are effectively overriding any native global implementation in user-code to unenv/node.

You make a really good point, Pooya! It's funny how we sometimes get caught up in not seeing the obvious.

I think the right thing here is to move these injects into the cloudflare preset because workerd doesn't offer Performance* APIs.

I'm not yet convinced that we need a dedicated wrangler preset. I'm hoping we really don't have to do that.

Would you be ok with me updating the PR by moving these injects into preset/cloudflare?

Copy link
Member

@pi0 pi0 Jun 18, 2024

Choose a reason for hiding this comment

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

I want to use same cloudflare preset in Nitro builds as well and users would face similar implicit opt-in to Node.js variant of Performance* variants if we do this for them (inject Web vs Node version)

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I see what you mean. The issue you are bringing up is that Node's Performance* API is not 100% compatible with Web's Performance* and if someone relies on globals we'd be making the choice for them, which might not be good. This is quite messy. How big is the difference between the two?

@jasnell as a node core contributor, have you seen this become a problem in the node community? how do people deal with these compat issues?

Copy link
Member

@pi0 pi0 Jun 18, 2024

Choose a reason for hiding this comment

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

Difference is:

  • In node impl, we always implement class and won't fall back to globalThis.* natives if available, in Web we do
  • Some classes only allow constructor via polyfills and not with natives and we need it internally between polyfills
  • Node.js classes have additional props

The first is solvable somehow but I'm more worried about the design decisions we hare making for unenv. For me unless explicitly we are adding "node.js" compatibility, we should strictly stick with Web standards spec. Only for wrangler usage that uses unenv as Node.js compatibility layer it clearly makes sense but for generic polyfill, it breaks the concept.

I am thinking to expose new API from unenv that we can have flags like nodeCompat to make difference behavior.


In the meantime, let's move these to cloudflare as you suggested (we can have web for nodeless + node for cloudflare that overrides nodeless) 👍🏼 this PR seems unnecessarily being blocked.

Copy link

Choose a reason for hiding this comment

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

we do have performance.now() and performance.timeOrigin in workers. The impl in Node.js definitely has a bunch of Node.js specific stuff. It hasn't been too much of a problem in the ecosystem but it definitely warrants caution.

},

polyfill: [
Expand Down
100 changes: 100 additions & 0 deletions src/runtime/node/perf_hooks/$cloudflare.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,100 @@
import type nodePerfHooks from "node:perf_hooks";

export {
Performance,
PerformanceEntry,
PerformanceMark,
PerformanceMeasure,
PerformanceObserverEntryList,
PerformanceObserver,
PerformanceResourceTiming,
constants,
createHistogram,
monitorEventLoopDelay,
} from "./index";

import {
Performance,
PerformanceEntry,
PerformanceMark,
PerformanceMeasure,
PerformanceObserverEntryList,
PerformanceObserver,
PerformanceResourceTiming,
constants,
createHistogram,
monitorEventLoopDelay,
performance as unenvPerformance,
} from "./index";

// The following is an unusual way to access the original/unpatched globalThis.performance.
// This is needed to get hold of the real performance object before any of the unenv polyfills are
// applied via `inject` or `polyfill` config in presets.
//
// This code relies on the that rollup/esbuild/webpack don't evaluate string concatenation
// so they don't recognize the below as `globalThis.performance` which they would try to rewrite
// into unenv/runtime/node/perf_hooks, thus creating a circular dependency, and breaking this polyfill.
const workerdGlobalPerformance = (globalThis as any)[
"perf" + "ormance"
] as typeof nodePerfHooks.performance;

// reuse unenv's polyfill, but since preserve globalThis.performance identity
// we use `.bind(unenvPerformance)` here to preserve the `this` for all delegated method calls
export const performance = Object.assign(workerdGlobalPerformance, {
// @ts-expect-error undocumented public API
addEventListener: unenvPerformance.addEventListener.bind(unenvPerformance),
clearMarks: unenvPerformance.clearMarks.bind(unenvPerformance),
clearMeasures: unenvPerformance.clearMeasures.bind(unenvPerformance),
clearResourceTimings:
unenvPerformance.clearResourceTimings.bind(unenvPerformance),
// @ts-expect-error undocumented public API
dispatchEvent: unenvPerformance.dispatchEvent.bind(unenvPerformance),
eventLoopUtilization:
unenvPerformance.eventLoopUtilization.bind(unenvPerformance),
getEntries: unenvPerformance.getEntries.bind(unenvPerformance),
getEntriesByName: unenvPerformance.getEntriesByName.bind(unenvPerformance),
getEntriesByType: unenvPerformance.getEntriesByType.bind(unenvPerformance),
mark: unenvPerformance.mark.bind(unenvPerformance),
markResourceTiming:
// @ts-expect-error undocumented public API
unenvPerformance.markResourceTiming.bind(unenvPerformance),
measure: unenvPerformance.measure.bind(unenvPerformance),
nodeTiming: { ...unenvPerformance.nodeTiming },
onresourcetimingbufferfull:
// @ts-expect-error undocumented public API
typeof unenvPerformance.onresourcetimingbufferfull === "function"
? // @ts-expect-error undocumented public API
unenvPerformance.onresourcetimingbufferfull.bind(unenvPerformance)
: // @ts-expect-error undocumented public API
unenvPerformance.onresourcetimingbufferfull,
removeEventListener:
// @ts-expect-error undocumented public API
unenvPerformance.removeEventListener.bind(unenvPerformance),
setResourceTimingBufferSize:
unenvPerformance.setResourceTimingBufferSize.bind(unenvPerformance),
timerify: unenvPerformance.timerify.bind(unenvPerformance),
toJSON: unenvPerformance.toJSON.bind(unenvPerformance),
});

export default {
/**
* manually unroll unenv-polyfilled-symbols to make it tree-shakeable
*/
Performance,
PerformanceEntry,
PerformanceMark,
PerformanceMeasure,
// @ts-expect-error TODO: resolve type-mismatch between web and node PerformanceObserverEntryList
PerformanceObserverEntryList,
PerformanceObserver,
// @ts-expect-error TODO: resolve type-mismatch between web and node PerformanceObserverEntryList
PerformanceResourceTiming,
constants,
createHistogram,
monitorEventLoopDelay,

/**
* manually unroll workerd-polyfilled-symbols to make it tree-shakeable
*/
performance,
} satisfies typeof nodePerfHooks;
66 changes: 64 additions & 2 deletions src/runtime/node/perf_hooks/internal/constants.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,64 @@
import type perf_hooks from "node:perf_hooks";

export const constants: typeof perf_hooks.constants = {
// captured from Node.js v22.3.0 using
// Object.getOwnPropertyDescriptors(require('perf_hooks').constants)
const constants: typeof perf_hooks.constants = Object.create(null, {
NODE_PERFORMANCE_ENTRY_TYPE_GC: {
value: 0,
enumerable: false,
},
NODE_PERFORMANCE_ENTRY_TYPE_HTTP: {
value: 1,
enumerable: false,
},
NODE_PERFORMANCE_ENTRY_TYPE_HTTP2: {
value: 2,
enumerable: false,
},
NODE_PERFORMANCE_ENTRY_TYPE_NET: {
value: 3,
enumerable: false,
},
NODE_PERFORMANCE_ENTRY_TYPE_DNS: {
value: 4,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_TIME_ORIGIN_TIMESTAMP: {
value: 0,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_TIME_ORIGIN: {
value: 1,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_ENVIRONMENT: {
value: 2,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_NODE_START: {
value: 3,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_V8_START: {
value: 4,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_LOOP_START: {
value: 5,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_LOOP_EXIT: {
value: 6,
enumerable: false,
},
NODE_PERFORMANCE_MILESTONE_BOOTSTRAP_COMPLETE: {
value: 7,
enumerable: false,
},
});

// add enumerable properties
Object.assign(constants, {
NODE_PERFORMANCE_GC_MAJOR: 4,
NODE_PERFORMANCE_GC_MINOR: 1,
NODE_PERFORMANCE_GC_INCREMENTAL: 8,
Expand All @@ -12,4 +70,8 @@ export const constants: typeof perf_hooks.constants = {
NODE_PERFORMANCE_GC_FLAGS_ALL_AVAILABLE_GARBAGE: 16,
NODE_PERFORMANCE_GC_FLAGS_ALL_EXTERNAL_MEMORY: 32,
NODE_PERFORMANCE_GC_FLAGS_SCHEDULE_IDLE: 64,
};
});

Object.freeze(constants);

export { constants };
41 changes: 39 additions & 2 deletions src/runtime/node/perf_hooks/internal/performance.ts
Original file line number Diff line number Diff line change
Expand Up @@ -16,6 +16,24 @@ export {
PerformanceObserverEntryList,
} from "../../../web/performance/index";

// grabbed from Node.js v22.3.0 using:
// performance.nodeTiming
const nodeTiming = {
name: "node",
entryType: "node",
startTime: 0,
duration: 305_963.045_666,
nodeStart: 1.662_124_991_416_931_2,
v8Start: 44.762_125_015_258_79,
bootstrapComplete: 49.992_666_006_088_26,
environment: 46.754_665_970_802_31,
loopStart: 63.262_040_972_709_656,
loopExit: -1,
idleTime: 305_360.555_328,
// only present in Node.js 18.x
detail: undefined,
} satisfies Omit<perf_hooks.PerformanceNodeTiming, "toJSON">;

// Performance
export const Performance = class Performance
extends _Performance<perf_hooks.PerformanceEntry>
Expand All @@ -31,8 +49,11 @@ export const Performance = class Performance
throw createNotImplementedError("Performance.timerify");
}

get nodeTiming() {
return <perf_hooks.PerformanceNodeTiming>{};
get nodeTiming(): perf_hooks.PerformanceNodeTiming {
return {
...nodeTiming,
toJSON: () => nodeTiming,
};
}

eventLoopUtilization() {
Expand All @@ -44,6 +65,22 @@ export const Performance = class Performance
return entry as any;
}

markResourceTiming(
timingInfo: object,
requestedUrl: string,
initiatorType: string,
global: object,
cacheMode: string,
bodyInfo: object,
responseStatus: number,
deliveryType?: string,
): PerformanceResourceTiming {
// TODO: create a new PerformanceResourceTiming entry
// so that performance.getEntries, getEntriesByName, and getEntriesByType return it
// see: https://nodejs.org/api/perf_hooks.html#performancemarkresourcetimingtiminginfo-requestedurl-initiatortype-global-cachemode-bodyinfo-responsestatus-deliverytype
return new _PerformanceResourceTiming("");
}

measure(
measureName: string,
startOrMeasureOptions?: string | PerformanceMeasureOptions | undefined,
Expand Down