Skip to content

Add perfetto for rust - #7528

Open
jasnell wants to merge 1 commit into
mainfrom
jasnell/rust-perfetto
Open

jasnell wants to merge 1 commit into
mainfrom
jasnell/rust-perfetto

Conversation

@jasnell

@jasnell jasnell commented Sep 26, 2026

Copy link
Copy Markdown
Collaborator

Make perfetto available to rust

@jasnell
jasnell requested review from a team as code owners September 26, 2026 03:42
Comment thread src/rust/perfetto/lib.rs Outdated
@ask-bonk

ask-bonk Bot commented Sep 26, 2026 •

Copy link
Copy Markdown
Contributor

Since last review: 0 resolved, 0 still open, 0 new.
LGTM!

Not re-run: tests, docs, compat-flags, design-simplicity, kj-style, memory-safety, rust-first (no author changes in their files since the last review)


Reviewed commit: 563c979f · github run

@jasnell
jasnell force-pushed the jasnell/rust-perfetto branch from 9a8fe98 to 8cd9e9c Compare September 26, 2026 03:51
@codecov-commenter

codecov-commenter commented Sep 26, 2026 •

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 4.60251% with 228 lines in your changes missing coverage. Please review.
✅ Project coverage is 41.79%. Comparing base (3d28dc6) to head (563c979).
⚠️ Report is 7 commits behind head on main.

Files with missing lines Patch % Lines
src/rust/perfetto/lib.rs 1.91% 154 Missing ⚠️
src/rust/perfetto/enabled.rs 6.32% 74 Missing ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #7528      +/-   ##
==========================================
- Coverage   41.82%   41.79%   -0.03%     
==========================================
  Files        1015     1017       +2     
  Lines      292015   292254     +239     
  Branches    28198    28198              
==========================================
+ Hits       122133   122147      +14     
- Misses     153387   153613     +226     
+ Partials    16495    16494       -1     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@jasnell
jasnell marked this pull request as draft September 26, 2026 04:19
@jasnell
jasnell force-pushed the jasnell/rust-perfetto branch from 8cd9e9c to b28f3c0 Compare October 10, 2026 01:39
@jasnell
jasnell marked this pull request as ready for review October 10, 2026 01:40
Comment thread src/rust/perfetto/enabled.rs Outdated
Comment thread src/rust/perfetto/lib.rs
Comment thread src/rust/perfetto/test/lib-test.rs
)

selects.config_setting_group(
name = "really_use_rust_perfetto",

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

This is kind of funny but I don't quite get why we need this really option.

@npaun npaun left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Looks perfetto to me once the Bonk issues are addressed. My clanker also brings up the exact same concerns. No further concerns from me as a human.

@jasnell
jasnell force-pushed the jasnell/rust-perfetto branch 3 times, most recently from 0b2f792 to c490e23 Compare October 16, 2026 17:37
@clanky-the-clanker

clanky-the-clanker Bot commented Oct 16, 2026 •

Copy link
Copy Markdown

💬 Approved with comments — No correctness or security problems found. The only item is an INFO note about per-event locking and allocation in the enabled Rust tracing backend, which costs nothing unless a tracing session is active.

Findings: 1 info

The decision is advisory: this comment never approves, blocks or requests changes on the pull request.

🔧 Fix with your agent

Paste the following into your coding agent to investigate and fix these issues.

Fix the following code review issues found in pull request #7528.

Before making changes, review each issue and present a brief summary table:
- For each finding, state whether you agree, disagree, or need clarification
- If you disagree (e.g., the fix would require disproportionate changes
  for minimal ROI, or the finding is factually incorrect), explain why
- If you need to clarify something before deciding, ask those questions
- Then share your plan for which issues to tackle and in what order

## INFO (1)

### [Performance] Enabled Rust trace points take a process-wide mutex and allocate on every event, and counters recompute their track UUID each call. This costs nothing unless tracing is active, but it could cause contention if trace points are added to multi-threaded hot paths.
- **File:** src/rust/perfetto/enabled.rs:129

⚡ Performance: 💬 1 info

No hot-path regressions; one note on per-event cost while tracing is enabled

Recommendation: 💬 Optional Nits

  • [INFO] Enabled Rust trace points take a process-wide mutex and allocate on every event, and counters recompute their track UUID each call. This costs nothing unless tracing is active, but it could cause contention if trace points are added to multi-threaded hot paths. (src/rust/perfetto/enabled.rs:129)
View full review
  • src/rust/perfetto/enabled.rs:90-94, 129-142: Every emitted event takes the SDK's process-wide lock, as the doc comment in lib.rs says. It also builds a fresh SdkContext and allocates for the extras. emit_counter recomputes the counter track UUID on every call. This only costs anything while a tracing session has the category enabled. The disabled path is two atomic loads.
  • Optional: keep Rust trace points off multi-threaded hot paths, as lib.rs already advises. If counters become frequent, cache the counter track UUID per call site.

✅ All other checks passed

🔒 Security: ✅ LGTM

This is internal tracing plumbing: a patch to the Perfetto build, a Rust SDK wrapper and a cxx bridge. None of it is reachable from user input.


📖 AGENTS.md: ✅ LGTM

The AGENTS.md updates in the root, src/rust/ and src/workerd/util/ match the new crate and build flag.


📋 Code Quality: ✅ LGTM

The Bazel select gating, the cfg-gated enabled and disabled backends, and the tests are consistent. //src/workerd/util:perfetto includes <workerd/rust/perfetto/ffi.rs.h> and depends only on //src/rust/perfetto. Other C++ targets do the same: async-trace depends on //src/rust/async-trace, and bench.c++ includes histogram/ffi.rs.h the same way. wd_rust_crate adds the bridge library to the crate's link_deps.


📝 Documentation: ✅ LGTM

The documentation updates in docs/v8-updates.md and the crate's doc comments match the change.


Reviewed commit: 563c979f

Make perfetto available to rust
@jasnell
jasnell force-pushed the jasnell/rust-perfetto branch from c490e23 to 563c979 Compare October 16, 2026 21:03
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.

3 participants