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

Application profiling #8695

Merged

Conversation

marcins
Copy link
Contributor

@marcins marcins commented Dec 16, 2022

↪️ Pull Request

This is an implementation of the Tracing RFC - more detail on the background and motivations is in #8588. The main differences from the RFC is that the feature is called "application profiling" (and the existing CPU profiling is referred to as "Sampling Profiling").

Instrumentation has been added for:

  • high-level phases - building, bundling, packaging
  • transform (per-plugin)
  • Babel transform (per-babel plugin visitor)
  • bundling (bundle / optimize)
  • packaging (package / optimize)
  • other plugin types (resolving, naming, etc..)

🚨 Test instructions

An app profile can be generated by starting Parcel with --profile-application.

✔️ PR Todo

  • Inject the profiler for plugins
  • Create a low-overhead instrumentation method
  • Added/updated unit tests for this change
  • Filled out test instructions (In case there aren't any unit tests)
  • Included links to related issues/PRs

See also documentation PR: parcel-bundler/website#1075

packages/core/core/src/PackagerRunner.js Outdated Show resolved Hide resolved
packages/core/core/src/PackagerRunner.js Outdated Show resolved Hide resolved
packages/core/core/src/Transformation.js Outdated Show resolved Hide resolved
packages/core/core/src/requests/PathRequest.js Outdated Show resolved Hide resolved
packages/core/profiler/src/ApplicationProfiler.js Outdated Show resolved Hide resolved
@lettertwo lettertwo self-requested a review December 16, 2022 18:36
@marcins marcins force-pushed the mszczepanski/application-profiling branch from 9809022 to 66fffa1 Compare March 2, 2023 22:00
@marcins
Copy link
Contributor Author

marcins commented Mar 2, 2023

I've finally gotten to addressing the PR feedback from late last year. The main changes have been:

  • rebasing on latest v2 branch
  • “injecting” a PluginApplicationProfiler similar to PluginLogger for plugins to ensure the same instance is used
    instrumenting the remaining plugin types in core

What’s still missing:

  • adding some test coverage to the new functionality

@marcins marcins marked this pull request as ready for review March 8, 2023 23:25
Copy link
Member

@lettertwo lettertwo left a comment

Choose a reason for hiding this comment

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

😍

@marcins marcins changed the title [WIP/Draft]: Application profiling Application profiling Mar 16, 2023
@lettertwo lettertwo self-requested a review March 30, 2023 16:10
@devongovett
Copy link
Member

This will be a really useful feature! My only question is around naming. I'm not sure what "application" refers to here. It's not really profiling the user's application so much as it is profiling Parcel. It seems more like a "trace" to me. Maybe we could have --profile and --trace as flags, and trace or tracer as the object passed to plugins? What do you think?

Also rename:
- @parcel/reporter-application-profiler -> @parcel/reporter-tracer
- PluginApplicationProfiler -> PluginTracer
- cli option `--profile-application` -> `--trace`
- PluginOptions.applicationProfiler -> PluginOptions.tracer
@lettertwo
Copy link
Member

Renamed ApplicationProfiler -> Tracer

Also renamed:

  • @parcel/reporter-application-profiler -> @parcel/reporter-tracer
  • PluginApplicationProfiler -> PluginTracer
  • cli option --profile-application -> --trace
  • PluginOptions.applicationProfiler -> PluginOptions.tracer

@lettertwo
Copy link
Member

@devongovett I think the rename is good, but with one small point of confusion: The SamplingProfiler has an accompanying Trace that is now very similar in name to Tracer (formerly called ApplicationProfiler. Should we rename Trace to something that ties it to SamplingProfiler?

@devongovett
Copy link
Member

That's not exposed externally to parcel though right? If not then doesn't seem like too big a deal.

@@ -11,6 +11,7 @@ export type FarmOptions = {
workerPath?: FilePath,
backend: BackendType,
shouldPatchConsole?: boolean,
shouldProfileApplication?: boolean,
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
shouldProfileApplication?: boolean,
shouldTrace?: boolean,

@@ -74,6 +76,28 @@ export default async function babel7(
},
};

if (tracer.enabled) {
config.wrapPluginVisitorMethod = (
Copy link
Member

Choose a reason for hiding this comment

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

Does this wrap visiting each individual AST node, or just once for each plugin? How expensive is that?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Does this wrap visiting each individual AST node, or just once for each plugin? How expensive is that?

I just ran two full production Jira builds back-to-back, one with tracing & one without (both with --no-cache). The timings were 18m 29s without tracing, and 19m 1s with tracing (on an M1 Mac). So minimal overhead for tracing in general.

The trace file produced is approximately 1.4GB - can't load it directly in the Perfetto UI (the WASM version), but works fine via the trace_processor CLI tool.

There are approximately 2.9m Babel transform entries - so yes, my understanding is that it wraps every visitor call for every AST type, but as you can see the overhead of this is relatively minimal.

@devongovett devongovett merged commit 482d23a into parcel-bundler:v2 May 12, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants