Add profiler-cli for querying profiles#5963
Conversation
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #5963 +/- ##
==========================================
- Coverage 85.34% 83.81% -1.53%
==========================================
Files 318 328 +10
Lines 31922 34253 +2331
Branches 8834 9580 +746
==========================================
+ Hits 27244 28710 +1466
- Misses 4246 5115 +869
+ Partials 432 428 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
This is ready for review! |
Was too fast 😓 |
canova
force-pushed
the
pq-cli
branch
3 times, most recently
from
90b59aa to
11d68ca
Compare
| 'function-include filter requires a non-empty filter string.' | ||
| ); | ||
| } | ||
| const funcIndexes = new Set(filter.split(',').map(Number)); |
There was a problem hiding this comment.
surprised we have a comma-separated string of numbers here - we have proper arrays for things like merge-call-node, why not here?
There was a problem hiding this comment.
It's mainly because the filter-samples transform accepts a string as a filter, and the idea was to make it more generic so it can accept a bunch of filterType as an argument. But it kinda restricts us in this case:
profiler/src/types/transforms.ts
Lines 371 to 381 in 8c0afb1
But that's not necessarily a restriction that we have to follow, we can always change it something like this instead:
'filter-samples':
| { readonly type: 'filter-samples'; readonly filterType: 'marker-search' | 'outside-marker'; readonly filter: string }
| { readonly type: 'filter-samples'; readonly filterType: 'function-include' | 'stack-prefix'; readonly funcIndexes: number[] }
| { readonly type: 'filter-samples'; readonly filterType: 'stack-suffix'; readonly funcIndex: number }I don't know if we should do it here in a follow-up as it wouldn't need any upgrader, let me know what you think.
There was a problem hiding this comment.
Does it even need to be a filter-samples transform? All transforms can filter samples.
There was a problem hiding this comment.
Well yeah, technically they can all filter samples, but currently we only have two transforms that touch the samples table: drop-function and filter-samples. The others usually update the stack table, frame table etc. That's why I think we had this distinction initially. When we were adding the filter-samples transform for the "drop samples outside of marker xyz" functionality, we wanted to make this a bit more generic so we can have more filters to be able to filter the samples. I don't think it makes a huge difference though.
| } | ||
| } | ||
| } | ||
| // Memoize bottom-up: does this stack contain any frame for funcIndex? |
There was a problem hiding this comment.
I think this function is computing a bunch of things that we already have code to compute. Haven't reviewed in detail yet.
There was a problem hiding this comment.
Yeah, I reviewed that one more time and improved it. Put it as a separate commit so it's easier to review: d1ebfbb
| } | ||
| } | ||
|
|
||
| // Assembly annotation |
There was a problem hiding this comment.
We don't really have enough information to make Assembly annotation work properly. We only have the mapping between source line and assembly instruction for instructions that were sampled. The symbolication API doesn't give us this information for the rest of the instructions yet.
There was a problem hiding this comment.
Yeah, but I wanted to do exactly what assembly view in the browser does, and I believe this behavior matches it. When I look at the assembly view in the browser I only see a single line in there with all the sample information summed up. And I use the same selectors to fetch this information. So I think it makes sense to keep them in sync. WDYT?
| * License, v. 2.0. If a copy of the MPL was not distributed with this | ||
| * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ | ||
|
|
||
| export type Slice = { |
There was a problem hiding this comment.
This whole file could use some comments. I wrote this code but I don't like it very much - and that includes the name! It's supposed to make a tree representation of the information you'd get from looking at a visual CPU usage graph, by applying a threshold operation to the entire time series, and then gradually increasing the threshold and seeing where "above water level" things split into separate islands. I suppose we can land it without comments and then I'll make a PR to add them
There was a problem hiding this comment.
Ah yeah, that's a good point. I kept it as is for now.
canova
force-pushed
the
pq-cli
branch
2 times, most recently
from
aa25d72 to
d1ebfbb
Compare
|
Thanks for the first look @mstange! I merged the "idle filtering" PR and rebased this PR on top of the recent main, so you'll see 2 less commits here. |
| /** | ||
| * Per-thread sizes of each filter "push group". One `filter push` adds one | ||
| * entry whose value is the number of Redux transforms it dispatched (e.g. | ||
| * `--merge f-1,f-2` → 2). `filter pop N` removes N groups, popping the |
There was a problem hiding this comment.
We should add a MergeSet transform :)
There was a problem hiding this comment.
Yeah, that would be nice to have! I added a FIXME below that comment.
| /** | ||
| * Override the symbolServerUrl field of the current URL state. `symbolServerUrl` | ||
| * has no dedicated action (the reducer is a read-only pass-through), so the | ||
| * only way to change it is to replace the whole UrlState via UPDATE_URL_STATE. | ||
| */ | ||
| function overrideSymbolServerUrl(store: Store, symbolServerUrl: string): void { | ||
| const current = getUrlState(store.getState()); | ||
| const next: UrlState = { ...current, symbolServerUrl }; | ||
| store.dispatch(updateUrlState(next)); | ||
| } |
There was a problem hiding this comment.
Looks like something we should fix properly as a follow-up.
There was a problem hiding this comment.
Yeah, added a FIXME here too.
| * Maps marker handles (like "m-1", "m-2") to (threadIndex, markerIndex) pairs. | ||
| * This provides a user-friendly way to reference markers in the CLI. | ||
| * | ||
| * Since each thread has its own marker list, we need to store both the thread | ||
| * index and the marker index to uniquely identify a marker. |
There was a problem hiding this comment.
It would probably be better to just have the thread index in the marker handle, and the actual marker index so that they're stable across multiple loads of the same profile.
There was a problem hiding this comment.
Is your suggestion something like m-<thread-handle>-<marker-index>?
I guess it would work, but it'll produce longer marker handles instead of nice short ones. But agents don't care too much about those, so maybe that's fine. It would be a bit more work for humans.
| * TimestampManager creates compact, hierarchical names for timestamps. | ||
| * | ||
| * Example names for range [1000, 2000]: | ||
| * - 1000 → "ts-0" (range start) | ||
| * - 2000 → "ts-Z" (range end) | ||
| * - 1500 → "ts-K" (middle of range) | ||
| * - 1000.1 → "ts-04" (between ts-0 and ts-1, drills into ts-0's subrange) | ||
| * - 500 → "ts<0K" (before range start, in first bucket before-range) | ||
| * - 2500 → "ts>0K" (after range end, in first bucket after-range) |
There was a problem hiding this comment.
I came up with this scheme but I guess we'll find out whether it's valuable or just cute and overengineered.
There was a problem hiding this comment.
Hehe, so far I don't necessarily see them being used in some example sessions I had. But let's see.
| export type CallTreeScoringStrategy = | ||
| | 'exponential-0.95' // totalPercentage * (0.95 ^ depth) - slow decay | ||
| | 'exponential-0.9' // totalPercentage * (0.9 ^ depth) - medium decay | ||
| | 'exponential-0.8' // totalPercentage * (0.8 ^ depth) - fast decay | ||
| | 'harmonic-0.1' // totalPercentage / (1 + 0.1 * depth) - very slow | ||
| | 'harmonic-0.5' // totalPercentage / (1 + 0.5 * depth) - medium | ||
| | 'harmonic-1.0' // totalPercentage / (1 + depth) - standard harmonic | ||
| | 'percentage-only'; // totalPercentage - no depth penalty |
There was a problem hiding this comment.
Should look at slimming this down
There was a problem hiding this comment.
Yeah. Another thing is that I haven't seen them being used at all so far.
mstange
left a comment
mstange
left a comment
There was a problem hiding this comment.
I think this is fine to land in this state. The comments I wrote on this PR are all things we can address in follow ups. And I expect us to keep tweaking things.
Previously only filtering by marker was possible. This patch adds some more filtering options that will only be used in the cli for now. They are not exposed in the profiler web frontend.
It will be used later by the profiler-cli.
canova
left a comment
canova
left a comment
There was a problem hiding this comment.
Thanks for the review! Updated the PR.
| /** | ||
| * Per-thread sizes of each filter "push group". One `filter push` adds one | ||
| * entry whose value is the number of Redux transforms it dispatched (e.g. | ||
| * `--merge f-1,f-2` → 2). `filter pop N` removes N groups, popping the |
There was a problem hiding this comment.
Yeah, that would be nice to have! I added a FIXME below that comment.
| /** | ||
| * Override the symbolServerUrl field of the current URL state. `symbolServerUrl` | ||
| * has no dedicated action (the reducer is a read-only pass-through), so the | ||
| * only way to change it is to replace the whole UrlState via UPDATE_URL_STATE. | ||
| */ | ||
| function overrideSymbolServerUrl(store: Store, symbolServerUrl: string): void { | ||
| const current = getUrlState(store.getState()); | ||
| const next: UrlState = { ...current, symbolServerUrl }; | ||
| store.dispatch(updateUrlState(next)); | ||
| } |
There was a problem hiding this comment.
Yeah, added a FIXME here too.
| * Maps marker handles (like "m-1", "m-2") to (threadIndex, markerIndex) pairs. | ||
| * This provides a user-friendly way to reference markers in the CLI. | ||
| * | ||
| * Since each thread has its own marker list, we need to store both the thread | ||
| * index and the marker index to uniquely identify a marker. |
There was a problem hiding this comment.
Is your suggestion something like m-<thread-handle>-<marker-index>?
I guess it would work, but it'll produce longer marker handles instead of nice short ones. But agents don't care too much about those, so maybe that's fine. It would be a bit more work for humans.
| * TimestampManager creates compact, hierarchical names for timestamps. | ||
| * | ||
| * Example names for range [1000, 2000]: | ||
| * - 1000 → "ts-0" (range start) | ||
| * - 2000 → "ts-Z" (range end) | ||
| * - 1500 → "ts-K" (middle of range) | ||
| * - 1000.1 → "ts-04" (between ts-0 and ts-1, drills into ts-0's subrange) | ||
| * - 500 → "ts<0K" (before range start, in first bucket before-range) | ||
| * - 2500 → "ts>0K" (after range end, in first bucket after-range) |
There was a problem hiding this comment.
Hehe, so far I don't necessarily see them being used in some example sessions I had. But let's see.
| export type CallTreeScoringStrategy = | ||
| | 'exponential-0.95' // totalPercentage * (0.95 ^ depth) - slow decay | ||
| | 'exponential-0.9' // totalPercentage * (0.9 ^ depth) - medium decay | ||
| | 'exponential-0.8' // totalPercentage * (0.8 ^ depth) - fast decay | ||
| | 'harmonic-0.1' // totalPercentage / (1 + 0.1 * depth) - very slow | ||
| | 'harmonic-0.5' // totalPercentage / (1 + 0.5 * depth) - medium | ||
| | 'harmonic-1.0' // totalPercentage / (1 + depth) - standard harmonic | ||
| | 'percentage-only'; // totalPercentage - no depth penalty |
There was a problem hiding this comment.
Yeah. Another thing is that I haven't seen them being used at all so far.
| 'function-include filter requires a non-empty filter string.' | ||
| ); | ||
| } | ||
| const funcIndexes = new Set(filter.split(',').map(Number)); |
There was a problem hiding this comment.
Well yeah, technically they can all filter samples, but currently we only have two transforms that touch the samples table: drop-function and filter-samples. The others usually update the stack table, frame table etc. That's why I think we had this distinction initially. When we were adding the filter-samples transform for the "drop samples outside of marker xyz" functionality, we wanted to make this a bit more generic so we can have more filters to be able to filter the samples. I don't think it makes a huge difference though.
3 participants
Fixes #5953 (we can move follow-ups that we have there once this lands).
This PR is the rebased, squashed and split version of #5663. I also did some reviews on top and updated the code further to fix some issues.
(This PR currently depends on #5968. Please review that one first)
It adds a new command-line tool,
profiler-cli, for querying Firefox Profiler profiles from a terminal. The CLI is published as a separate npm package (@firefox-devtools/profiler-cli, alpha) and is the intended entry point for most usersWhat's in here:
New
src/profile-query/library: ReusableProfileQuerierclass that loads a profile (file,profiler.firefox.comURL, or share link) and exposes methods for profile/thread info, samples (top-down / bottom-up / hot functions), markers, functions, zoom (view range) stack, and per-thread filter stack. All query methods return structured result objects; formatting lives in the CLI layer.New
profiler-cli/package: Node CLI (profiler-cli, short aliaspq) with a persistent daemon architecture so subsequent commands against the same profile are fast. Sessions are stored under~/.profiler-cli/.How to do manual testing locally:
yarn installandyarn build-profiler-clialias profiler-cli="node $(pwd)/profiler-cli/dist/profiler-cli.js" && alias pq="node $(pwd)/profiler-cli/dist/profiler-cli.js"pqandprofiler-cliin this terminal session.