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!: use inspector for time profiles #227

Open
wants to merge 11 commits into
base: main
Choose a base branch
from
18 changes: 11 additions & 7 deletions ts/src/index.ts
Original file line number Diff line number Diff line change
Expand Up @@ -37,12 +37,16 @@ export const heap = {

// If loaded with --require, start profiling.
if (module.parent && module.parent.id === 'internal/preload') {
const stop = time.start();
process.on('exit', () => {
// The process is going to terminate imminently. All work here needs to
// be synchronous.
const profile = stop();
const buffer = encodeSync(profile);
writeFileSync(`pprof-profile-${process.pid}.pb.gz`, buffer);
time.start().then(stop => {
process.on('exit', async () => {
// The process is going to terminate imminently. All work here needs to
// be synchronous.

// TODO: this code no longer works because stop() cannot be run synchronously. Maybe
// beforeExit event would be a decent middleground, or can run this in a signal handler.
aabmass marked this conversation as resolved.
Show resolved Hide resolved
const profile = await stop();
const buffer = encodeSync(profile);
writeFileSync(`pprof-profile-${process.pid}.pb.gz`, buffer);
});
});
}
113 changes: 113 additions & 0 deletions ts/src/time-profiler-inspector.ts
Original file line number Diff line number Diff line change
@@ -0,0 +1,113 @@
/**
* Copyright 2018 Google Inc. All Rights Reserved.
aabmass marked this conversation as resolved.
Show resolved Hide resolved
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

import {TimeProfile, TimeProfileNode} from './v8-types';
import * as inspector from 'node:inspector';

const session = new inspector.Session();
session.connect();
psx95 marked this conversation as resolved.
Show resolved Hide resolved

// Wrappers around inspector functions
export function startProfiling(): Promise<void> {
return new Promise<void>((resolve, reject) => {
session.post('Profiler.enable', err => {
if (err !== null) {
reject(err);
aabmass marked this conversation as resolved.
Show resolved Hide resolved
}
session.post('Profiler.start', err => {
aabmass marked this conversation as resolved.
Show resolved Hide resolved
if (err !== null) {
reject(err);
aabmass marked this conversation as resolved.
Show resolved Hide resolved
}
resolve();
});
});
});
}

export function stopProfiling(): Promise<TimeProfile> {
// return profiler.timeProfiler.stopProfiling(runName, includeLineInfo || false);
return new Promise<TimeProfile>((resolve, reject) => {
session.post('Profiler.stop', (err, {profile}) => {
if (err !== null) {
reject(err);
aabmass marked this conversation as resolved.
Show resolved Hide resolved
}
resolve(translateToTimeProfile(profile));
});
});
}

function translateToTimeProfile(
profile: inspector.Profiler.Profile
): TimeProfile {
const root: inspector.Profiler.ProfileNode | undefined = profile.nodes[0];
// Not sure if this could ever happen...
if (root === undefined) {
return {
endTime: profile.endTime,
startTime: profile.startTime,
topDownRoot: {
children: [],
hitCount: 0,
scriptName: '',
},
};
}

const nodesById: {[key: number]: inspector.Profiler.ProfileNode} = {};
profile.nodes.forEach(node => (nodesById[node.id] = node));

function translateNode({
hitCount,
children,
callFrame: {columnNumber, functionName, lineNumber, scriptId, url},
}: inspector.Profiler.ProfileNode): TimeProfileNode {
const parsedScriptId = parseInt(scriptId);
return {
name: functionName,
scriptName: url,
aabmass marked this conversation as resolved.
Show resolved Hide resolved

// Add 1 because these are zero-based
columnNumber: columnNumber + 1,
lineNumber: lineNumber + 1,

hitCount: hitCount ?? 0,
scriptId: Number.isNaN(parsedScriptId) ? 0 : parsedScriptId,
children:
children?.map(childId => translateNode(nodesById[childId])) ?? [],
};
}

return {
endTime: profile.endTime,
startTime: profile.startTime,
topDownRoot: translateNode(root),
};
}

export function setSamplingInterval(intervalMicros: number): Promise<void> {
return new Promise<void>((resolve, reject) => {
session.post(
'Profiler.setSamplingInterval',
{interval: intervalMicros},
err => {
if (err !== null) {
reject(err);
aabmass marked this conversation as resolved.
Show resolved Hide resolved
}
resolve();
}
);
});
}
25 changes: 10 additions & 15 deletions ts/src/time-profiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -22,7 +22,7 @@ import {
setSamplingInterval,
startProfiling,
stopProfiling,
} from './time-profiler-bindings';
} from './time-profiler-inspector';

let profiling = false;

Expand All @@ -49,40 +49,35 @@ export interface TimeProfilerOptions {
}

export async function profile(options: TimeProfilerOptions) {
const stop = start(
const stop = await start(
options.intervalMicros || DEFAULT_INTERVAL_MICROS,
options.name,
options.sourceMapper,
options.lineNumbers
options.sourceMapper
);
await delay(options.durationMillis);
return stop();
return await stop();
}

export function start(
export async function start(
Copy link
Member Author

Choose a reason for hiding this comment

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

This now has to be async which is another breaking change.

Copy link
Contributor

Choose a reason for hiding this comment

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

There's a chance (depending on underlying implementation) this will result in a fix for googleapis/cloud-profiler-nodejs#683 (event loop blocked at start of profile collection).

This would be a good change if it does that (though needing to update agent code will be a bummer)

Copy link
Member Author

Choose a reason for hiding this comment

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

Great! I will assume this fixes that issue and close it once this change gets pull into that repo.

intervalMicros: Microseconds = DEFAULT_INTERVAL_MICROS,
name?: string,
sourceMapper?: SourceMapper,
lineNumbers?: boolean
sourceMapper?: SourceMapper
) {
if (profiling) {
throw new Error('already profiling');
}

profiling = true;
const runName = name || `pprof-${Date.now()}-${Math.random()}`;
setSamplingInterval(intervalMicros);
await setSamplingInterval(intervalMicros);
// Node.js contains an undocumented API for reporting idle status to V8.
// This lets the profiler distinguish idle time from time spent in native
// code. Ideally this should be default behavior. Until then, use the
// undocumented API.
// See https://github.com/nodejs/node/issues/19009#issuecomment-403161559.
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(process as any)._startProfilerIdleNotifier();
startProfiling(runName, lineNumbers);
return function stop() {
await startProfiling();
return async function stop() {
profiling = false;
const result = stopProfiling(runName, lineNumbers);
const result = await stopProfiling();
// eslint-disable-next-line @typescript-eslint/no-explicit-any
(process as any)._stopProfilerIdleNotifier();
const profile = serializeTimeProfile(result, intervalMicros, sourceMapper);
Expand Down
10 changes: 6 additions & 4 deletions ts/test/test-time-profiler.ts
Original file line number Diff line number Diff line change
Expand Up @@ -17,7 +17,7 @@
import delay from 'delay';
import * as sinon from 'sinon';
import * as time from '../src/time-profiler';
import * as v8TimeProfiler from '../src/time-profiler-bindings';
import * as inspectorTimeProfiler from '../src/time-profiler-inspector';
import {timeProfile, v8TimeProfile} from './profiles-for-tests';

const assert = require('assert');
Expand Down Expand Up @@ -46,11 +46,13 @@ describe('Time Profiler', () => {
// eslint-disable-next-line @typescript-eslint/no-explicit-any
const sinonStubs: Array<sinon.SinonStub<any, any>> = [];
before(() => {
sinonStubs.push(sinon.stub(v8TimeProfiler, 'startProfiling'));
sinonStubs.push(sinon.stub(inspectorTimeProfiler, 'startProfiling'));
sinonStubs.push(
sinon.stub(v8TimeProfiler, 'stopProfiling').returns(v8TimeProfile)
sinon
.stub(inspectorTimeProfiler, 'stopProfiling')
.resolves(v8TimeProfile)
);
sinonStubs.push(sinon.stub(v8TimeProfiler, 'setSamplingInterval'));
sinonStubs.push(sinon.stub(inspectorTimeProfiler, 'setSamplingInterval'));
sinonStubs.push(sinon.stub(Date, 'now').returns(0));
});

Expand Down