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

Fix tracing problems #9622

Closed
wants to merge 4 commits into from
Closed
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 flow-libs/chrome-trace-event.js.flow
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,7 @@ declare module 'chrome-trace-event' {
begin(fields: Fields): void;
completeEvent(fields: Fields): void;
instantEvent(fields: Fields): void;
mkEventFunc(ph: string): (fields: Fields) => void;
flush(): void;
}
}
25 changes: 16 additions & 9 deletions packages/core/core/src/ParcelConfig.js
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@ import {
fromProjectPathRelative,
toProjectPathUnsafe,
} from './projectPath';
import {measureAsyncFunction} from '@parcel/profiler';

type GlobMap<T> = {[Glob]: T, ...};
type SerializedParcelConfig = {|
Expand Down Expand Up @@ -157,16 +158,22 @@ export default class ParcelConfig {
return Promise.all(plugins.map(p => this.loadPlugin<T>(p)));
}

async getResolvers(): Promise<Array<LoadedPlugin<Resolver<mixed>>>> {
if (this.resolvers.length === 0) {
throw await this.missingPluginError(
this.resolvers,
'No resolver plugins specified in .parcelrc config',
'/resolvers',
);
}
getResolvers(): Promise<Array<LoadedPlugin<Resolver<mixed>>>> {
return measureAsyncFunction(
'ParcelConfig::getResolvers',
'resolve',
async () => {
if (this.resolvers.length === 0) {
throw await this.missingPluginError(
this.resolvers,
'No resolver plugins specified in .parcelrc config',
'/resolvers',
);
}

return this.loadPlugins<Resolver<mixed>>(this.resolvers);
return this.loadPlugins<Resolver<mixed>>(this.resolvers);
},
);
}

_getValidatorNodes(filePath: ProjectPath): $ReadOnlyArray<ParcelPluginNode> {
Expand Down
4 changes: 3 additions & 1 deletion packages/core/core/src/ReporterRunner.js
Original file line number Diff line number Diff line change
Expand Up @@ -99,7 +99,7 @@ export default class ReporterRunner {
try {
// To avoid an infinite loop we don't measure trace events, as they'll
// result in another trace!
if (event.type !== 'trace') {
if (!event.type.startsWith('trace')) {
measurement = tracer.createMeasurement(reporter.name, 'reporter');
}
await reporter.plugin.report({
Expand All @@ -113,12 +113,14 @@ export default class ReporterRunner {
});
} catch (reportError) {
INTERNAL_ORIGINAL_CONSOLE.error(reportError);
INTERNAL_ORIGINAL_CONSOLE.error(reportError.stack);
} finally {
measurement && measurement.end();
}
}
} catch (err) {
INTERNAL_ORIGINAL_CONSOLE.error(err);
INTERNAL_ORIGINAL_CONSOLE.error(err.stack);
}
}

Expand Down
31 changes: 23 additions & 8 deletions packages/core/core/src/Transformation.js
Original file line number Diff line number Diff line change
Expand Up @@ -356,8 +356,17 @@ export default class Transformation {
let inputAssets = [initialAsset];
let resultingAssets = [];
let finalAssets = [];
const pipelineTransformers = tracer.createMeasurement(
'Transformation::runPipeline - transformers',
'transform',
pipeline.id,
);
for (let transformer of pipeline.transformers) {
resultingAssets = [];
const transformerMeasurement = tracer.createMeasurement(
`${transformer.name} all_assets`,
'transform',
);
for (let asset of inputAssets) {
if (
asset.value.type !== initialType &&
Expand All @@ -373,13 +382,13 @@ export default class Transformation {
continue;
}

try {
const measurement = tracer.createMeasurement(
transformer.name,
'transform',
fromProjectPathRelative(initialAsset.value.filePath),
);
const measurement = tracer.createMeasurement(
transformer.name,
'transform',
fromProjectPathRelative(initialAsset.value.filePath),
);

try {
let transformerResults = await this.runTransformer(
pipeline,
asset,
Expand All @@ -390,11 +399,10 @@ export default class Transformation {
this.parcelConfig,
);

measurement && measurement.end();

for (let result of transformerResults) {
if (result instanceof UncommittedAsset) {
resultingAssets.push(result);
measurement?.end();
continue;
}
resultingAssets.push(
Expand Down Expand Up @@ -434,13 +442,20 @@ export default class Transformation {
}
}

measurement?.end();
transformerMeasurement?.end();
pipelineTransformers?.end();
throw new ThrowableDiagnostic({
diagnostic,
});
}

measurement && measurement.end();
}
transformerMeasurement?.end();
inputAssets = resultingAssets;
}
pipelineTransformers?.end();

// Make assets with ASTs generate unless they are CSS modules. This parallelizes generation
// and distributes work more evenly across workers than if one worker needed to
Expand Down
60 changes: 33 additions & 27 deletions packages/core/core/src/requests/ConfigRequest.js
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ import PublicConfig from '../public/Config';
import {optionsProxy} from '../utils';
import {getInvalidationHash} from '../assetUtils';
import {hashString, Hash} from '@parcel/rust';
import {PluginTracer} from '@parcel/profiler';
import {measureAsyncFunction, PluginTracer} from '@parcel/profiler';
import {requestTypes} from '../RequestTracker';
import {fromProjectPath, fromProjectPathRelative} from '../projectPath';
import {createBuildCache} from '../buildCache';
Expand Down Expand Up @@ -74,37 +74,43 @@ export type ConfigRequest = {
...
};

export async function loadPluginConfig<T: PluginWithLoadConfig>(
export function loadPluginConfig<T: PluginWithLoadConfig>(
loadedPlugin: LoadedPlugin<T>,
config: Config,
options: ParcelOptions,
): Promise<void> {
let loadConfig = loadedPlugin.plugin.loadConfig;
if (!loadConfig) {
return;
}
return measureAsyncFunction(
Copy link
Member

@devongovett devongovett Apr 5, 2024

Choose a reason for hiding this comment

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

what's the goal of measuring internal functions like this? This would be captured by the normal v8 profiler already. I thought the goal of tracing is to capture high level timings for user-facing things like plugins. If we start tracing internal things in Parcel core this gets muddier and harder for end users to understand.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

The goal is to capture high-level timings, so lower resolution than what the profiler will spit out (& lower or close to none overhead).

We could trace more internal bits though. It should not be granular.

I don't really care about the measurements added in this diff. More about the fixes.

For the measureAsyncFunction / measureFunction helpers ; they prevent mistakes where .end isn't currently called.

That was the case on packages/core/core/src/Transformation.js, where you can see I added measurement.end() calls that were previously not there. For those branches we had missing traces. Ideally we'd replace all measurement.end() calls with function wrappers, since then we can't not call .end().

'ConfigRequest::loadPluginConfig',
'building',
async () => {
let loadConfig = loadedPlugin.plugin.loadConfig;
if (!loadConfig) {
return;
}

try {
config.result = await loadConfig({
config: new PublicConfig(config, options),
options: new PluginOptions(
optionsProxy(options, option => {
config.invalidateOnOptionChange.add(option);
}),
),
logger: new PluginLogger({origin: loadedPlugin.name}),
tracer: new PluginTracer({
origin: loadedPlugin.name,
category: 'loadConfig',
}),
});
} catch (e) {
throw new ThrowableDiagnostic({
diagnostic: errorToDiagnostic(e, {
origin: loadedPlugin.name,
}),
});
}
try {
config.result = await loadConfig({
config: new PublicConfig(config, options),
options: new PluginOptions(
optionsProxy(options, option => {
config.invalidateOnOptionChange.add(option);
}),
),
logger: new PluginLogger({origin: loadedPlugin.name}),
tracer: new PluginTracer({
origin: loadedPlugin.name,
category: 'loadConfig',
}),
});
} catch (e) {
throw new ThrowableDiagnostic({
diagnostic: errorToDiagnostic(e, {
origin: loadedPlugin.name,
}),
});
}
},
);
}

const configKeyCache = createBuildCache();
Expand Down
119 changes: 61 additions & 58 deletions packages/core/core/src/requests/DevDepRequest.js
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
// @flow
import type {DependencySpecifier, SemverRange} from '@parcel/types';
import {measureAsyncFunction} from '@parcel/profiler';
import type ParcelConfig from '../ParcelConfig';
import type {
DevDepRequest,
Expand All @@ -26,75 +27,77 @@ import {requestTypes} from '../RequestTracker';
// paths and hashes.
const devDepRequestCache = new WeakMap();

export async function createDevDependency(
export function createDevDependency(
opts: InternalDevDepOptions,
requestDevDeps: Map<string, string>,
options: ParcelOptions,
): Promise<DevDepRequest> {
let {specifier, resolveFrom, additionalInvalidations} = opts;
let key = `${specifier}:${fromProjectPathRelative(resolveFrom)}`;

// If the request sent us a hash, we know the dev dep and all of its dependencies didn't change.
// Reuse the same hash in the response. No need to send back invalidations as the request won't
// be re-run anyway.
let hash = requestDevDeps.get(key);
if (hash != null) {
return {
specifier,
resolveFrom,
hash,
};
}
return measureAsyncFunction('createDevDependency', 'resolve', async () => {
let {specifier, resolveFrom, additionalInvalidations} = opts;
let key = `${specifier}:${fromProjectPathRelative(resolveFrom)}`;

let resolveFromAbsolute = fromProjectPath(options.projectRoot, resolveFrom);
// If the request sent us a hash, we know the dev dep and all of its dependencies didn't change.
// Reuse the same hash in the response. No need to send back invalidations as the request won't
// be re-run anyway.
let hash = requestDevDeps.get(key);
if (hash != null) {
return {
specifier,
resolveFrom,
hash,
};
}

// Ensure that the package manager has an entry for this resolution.
try {
await options.packageManager.resolve(specifier, resolveFromAbsolute);
} catch (err) {
// ignore
}
let resolveFromAbsolute = fromProjectPath(options.projectRoot, resolveFrom);

let invalidations = options.packageManager.getInvalidations(
specifier,
resolveFromAbsolute,
);
// Ensure that the package manager has an entry for this resolution.
try {
await options.packageManager.resolve(specifier, resolveFromAbsolute);
} catch (err) {
// ignore
}

let cached = devDepRequestCache.get(invalidations);
if (cached != null) {
return cached;
}
let invalidations = options.packageManager.getInvalidations(
specifier,
resolveFromAbsolute,
);

let invalidateOnFileChangeProject = [
...invalidations.invalidateOnFileChange,
].map(f => toProjectPath(options.projectRoot, f));

// It is possible for a transformer to have multiple different hashes due to
// different dependencies (e.g. conditional requires) so we must always
// recompute the hash and compare rather than only sending a transformer
// dev dependency once.
hash = await getInvalidationHash(
invalidateOnFileChangeProject.map(f => ({
type: 'file',
filePath: f,
})),
options,
);
let cached = devDepRequestCache.get(invalidations);
if (cached != null) {
return cached;
}

let devDepRequest: DevDepRequest = {
specifier,
resolveFrom,
hash,
invalidateOnFileCreate: invalidations.invalidateOnFileCreate.map(i =>
invalidateOnFileCreateToInternal(options.projectRoot, i),
),
invalidateOnFileChange: new Set(invalidateOnFileChangeProject),
invalidateOnStartup: invalidations.invalidateOnStartup,
additionalInvalidations,
};
let invalidateOnFileChangeProject = [
...invalidations.invalidateOnFileChange,
].map(f => toProjectPath(options.projectRoot, f));

// It is possible for a transformer to have multiple different hashes due to
// different dependencies (e.g. conditional requires) so we must always
// recompute the hash and compare rather than only sending a transformer
// dev dependency once.
hash = await getInvalidationHash(
invalidateOnFileChangeProject.map(f => ({
type: 'file',
filePath: f,
})),
options,
);

let devDepRequest: DevDepRequest = {
specifier,
resolveFrom,
hash,
invalidateOnFileCreate: invalidations.invalidateOnFileCreate.map(i =>
invalidateOnFileCreateToInternal(options.projectRoot, i),
),
invalidateOnFileChange: new Set(invalidateOnFileChangeProject),
invalidateOnStartup: invalidations.invalidateOnStartup,
additionalInvalidations,
};

devDepRequestCache.set(invalidations, devDepRequest);
return devDepRequest;
devDepRequestCache.set(invalidations, devDepRequest);
return devDepRequest;
});
}

export type DevDepSpecifier = {|
Expand Down
Loading
Loading