From 949b7b6d37c130c33a45b6374a459dae680d37bb Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 29 May 2026 15:49:03 +0200 Subject: [PATCH 1/2] fix(time-profiler): serialize before clearing the source mapper in stop() The legacy time profiler stop() called handleStopRestart()/ handleStopNoRestart() before serializing the profile. handleStopNoRestart() clears gSourceMapper (and disposes the profiler), so serialization ran with an undefined source mapper: transpiled frames (e.g. TypeScript) were left pointing at the generated .js files instead of being mapped back to their original sources. Move serialization ahead of the teardown so the source mapper is still set, matching stopV2(), which serializes inside the stopAndCollect callback while gSourceMapper is still defined. This restores the behaviour of 5.14.1, whose stop() serialized via stopAndCollect before teardown. Add a regression test asserting stop() passes the source mapper to serializeTimeProfile. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Matteo Collina --- ts/src/time-profiler.ts | 18 +++++++++++++----- ts/test/test-time-profiler.ts | 27 +++++++++++++++++++++++++++ 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/ts/src/time-profiler.ts b/ts/src/time-profiler.ts index 1c023c0d..5b161db4 100644 --- a/ts/src/time-profiler.ts +++ b/ts/src/time-profiler.ts @@ -168,12 +168,13 @@ export function stop( } const profile = gProfiler.stop(restart); - if (restart) { - handleStopRestart(); - } else { - handleStopNoRestart(); - } + // Serialize before tearing down the profiler state. handleStopNoRestart() + // clears gSourceMapper (and disposes gProfiler), so serializing afterwards + // would drop source-map resolution and leave transpiled frames pointing at + // the generated files instead of the original sources. This matches the + // ordering used by stopV2() below, which serializes inside the collect + // callback while gSourceMapper is still set. const serializedProfile = serializeTimeProfile( profile, gIntervalMicros, @@ -182,6 +183,13 @@ export function stop( generateLabels, lowCardinalityLabels, ); + + if (restart) { + handleStopRestart(); + } else { + handleStopNoRestart(); + } + return serializedProfile; } diff --git a/ts/test/test-time-profiler.ts b/ts/test/test-time-profiler.ts index 87c8e4a5..8b0b6fa8 100644 --- a/ts/test/test-time-profiler.ts +++ b/ts/test/test-time-profiler.ts @@ -18,6 +18,8 @@ import * as sinon from 'sinon'; import {time, getNativeThreadId} from '../src'; import {profileV2, stopV2} from '../src/time-profiler'; import * as v8TimeProfiler from '../src/time-profiler-bindings'; +import * as profileSerializer from '../src/profile-serializer'; +import {SourceMapper} from '../src/sourcemapper/sourcemapper'; import {timeProfile, v8TimeProfile} from './profiles-for-tests'; import {hrtime} from 'process'; import {Label, Profile} from 'pprof-format'; @@ -495,6 +497,31 @@ describe('Time Profiler', () => { sinon.assert.notCalled(timeProfilerStub.start); sinon.assert.calledOnce(timeProfilerStub.stop); }); + + it('should serialize with the source mapper still set when stopping', () => { + // Regression test: stop() used to tear down the profiler state (via + // handleStopNoRestart, which clears gSourceMapper) *before* serializing, + // so the source mapper passed to start() was dropped and transpiled + // frames were left pointing at the generated files instead of the + // original sources. + const sourceMapper = {} as unknown as SourceMapper; + const serializeStub = sinon + .stub(profileSerializer, 'serializeTimeProfile') + .returns(timeProfile); + try { + time.start({ + intervalMicros: PROFILE_OPTIONS.intervalMicros, + sourceMapper, + }); + time.stop(); + + sinon.assert.calledOnce(serializeStub); + // The third argument to serializeTimeProfile is the source mapper. + assert.strictEqual(serializeStub.getCall(0).args[2], sourceMapper); + } finally { + serializeStub.restore(); + } + }); }); describe('profileV2', () => { From 4ae3f05568b87d305665329f5b907f8afc10b441 Mon Sep 17 00:00:00 2001 From: Matteo Collina Date: Fri, 29 May 2026 18:22:18 +0200 Subject: [PATCH 2/2] address review: keep early restart, defer only no-restart teardown Per review feedback, the profiler restart should still happen before serialization so the cost of serialization is captured in the next profile. Only handleStopNoRestart() clears gSourceMapper, so only that path needs to run after serialization; handleStopRestart() stays early and does not touch the source mapper. Extend the regression test to also assert the source mapper is preserved on the restart (stop(true)) path. Assisted-by: Claude Code:claude-opus-4-8 Signed-off-by: Matteo Collina --- ts/src/time-profiler.ts | 21 ++++++++++++--------- ts/test/test-time-profiler.ts | 27 +++++++++++++++++++++++---- 2 files changed, 35 insertions(+), 13 deletions(-) diff --git a/ts/src/time-profiler.ts b/ts/src/time-profiler.ts index 5b161db4..d5b465b6 100644 --- a/ts/src/time-profiler.ts +++ b/ts/src/time-profiler.ts @@ -169,12 +169,13 @@ export function stop( const profile = gProfiler.stop(restart); - // Serialize before tearing down the profiler state. handleStopNoRestart() - // clears gSourceMapper (and disposes gProfiler), so serializing afterwards - // would drop source-map resolution and leave transpiled frames pointing at - // the generated files instead of the original sources. This matches the - // ordering used by stopV2() below, which serializes inside the collect - // callback while gSourceMapper is still set. + if (restart) { + // Restart the profiler *before* serializing so the cost of serialization is + // captured in the next profile. handleStopRestart() does not dispose of the + // source mapper, so the source-map resolution below is unaffected. + handleStopRestart(); + } + const serializedProfile = serializeTimeProfile( profile, gIntervalMicros, @@ -184,9 +185,11 @@ export function stop( lowCardinalityLabels, ); - if (restart) { - handleStopRestart(); - } else { + if (!restart) { + // Tear down *after* serializing: handleStopNoRestart() clears gSourceMapper + // (and disposes gProfiler), so serializing afterwards would drop source-map + // resolution and leave transpiled frames pointing at the generated files + // instead of the original sources. handleStopNoRestart(); } diff --git a/ts/test/test-time-profiler.ts b/ts/test/test-time-profiler.ts index 8b0b6fa8..54c5b856 100644 --- a/ts/test/test-time-profiler.ts +++ b/ts/test/test-time-profiler.ts @@ -503,21 +503,40 @@ describe('Time Profiler', () => { // handleStopNoRestart, which clears gSourceMapper) *before* serializing, // so the source mapper passed to start() was dropped and transpiled // frames were left pointing at the generated files instead of the - // original sources. + // original sources. The third argument to serializeTimeProfile is the + // source mapper; it must still be the one passed to start(). const sourceMapper = {} as unknown as SourceMapper; const serializeStub = sinon .stub(profileSerializer, 'serializeTimeProfile') .returns(timeProfile); try { + // no-restart path: handleStopNoRestart() clears gSourceMapper, so it + // must run after serialization. time.start({ intervalMicros: PROFILE_OPTIONS.intervalMicros, sourceMapper, }); time.stop(); - sinon.assert.calledOnce(serializeStub); - // The third argument to serializeTimeProfile is the source mapper. - assert.strictEqual(serializeStub.getCall(0).args[2], sourceMapper); + assert.strictEqual( + serializeStub.getCall(0).args[2], + sourceMapper, + 'source mapper dropped on stop()', + ); + + // restart path: the source mapper must be preserved here too. + serializeStub.resetHistory(); + time.start({ + intervalMicros: PROFILE_OPTIONS.intervalMicros, + sourceMapper, + }); + time.stop(true); + assert.strictEqual( + serializeStub.getCall(0).args[2], + sourceMapper, + 'source mapper dropped on stop(true)', + ); + time.stop(); // finalize: dispose the restarted profiler } finally { serializeStub.restore(); }