Weight results by perf event periods

CHANGELOG.md

@@ -11,6 +11,8 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
 
 * Load samply's version 75 profiles, including the JSLB container (`.jslb`, `.jslb.gz`).
 * `describe_profile` and `summary` list the events a profile contains.
+* Weight `top_functions`, `top_groups`, `call_tree`, `stacks_containing`, `folded_stacks`, `compare_profiles`, `compare_functions`, `source_for_function`, `asm_for_function`, and the `summary` rankings by perf event period when samply recorded periods. Outputs report `weighted`.
+* `describe_profile` and `summary` name the main perf event and each event's sampling mode.
 
 ### Fixed
 

README.md

@@ -28,6 +28,8 @@ Published to [crates.io](https://crates.io/crates/pollard).
 hardware counter instead.
 `top_groups` currently aggregates samples only.
 `describe_profile` lists the events a profile contains.
+When samply recorded perf event periods (`samply import --weight-by-period`), the query and drill-down tools add periods instead of counting samples, so their counts and percentages mean events such as cycles or cache misses.
+Their outputs report `weighted: true` in that case, and `compare_profiles` adds a `note` when only one side is weighted.
 
 See `docs/superpowers/specs/2026-04-28-pollard-design.md` for full details.
 See `docs/superpowers/specs/2026-05-06-view-presets-cookbook.md` for

docs/superpowers/specs/2026-09-26-perf-event-periods-design.md

@@ -0,0 +1,164 @@
+# Perf event periods from samply to pollard
+
+## Problem
+
+pollard counts every sample and every `Other event` marker as 1, because samply drops the perf event period when it imports `perf.data`.
+In frequency mode (`perf record -F`), perf adjusts the period per sample, so equal counts do not mean equal event totals, and count shares are biased toward code that ran while the period was small.
+The profile also does not name the event behind the samples track, so pollard can only call it `samples`.
+This spec covers sub-project B: samply records periods and event metadata, and pollard weights by them.
+It builds on sub-project A (`2026-09-25-shared-tables-format-design.md`), which makes pollard read samply's current output.
+
+## Evidence
+
+The findings below come from `samply import` of `perf record -e cycles,cache-misses,instructions,branch-misses` recordings made with `-F 999` and with `-c 100000`, on samply `da48ff40`.
+
+* Every main-event sample gets `weight` 1 at all three `add_sample` sites in `handle_main_event_sample` (`samply/src/linux_shared/converter.rs`): the thread, the per-CPU thread, and the combined CPU thread.
+* samply writes the `samples.weight` column unconditionally (`fxprof-processed-profile/src/sample_table.rs`), so its presence says nothing about how the weights were chosen.
+* Off-CPU samples get weight 1 whenever `sampling_is_time_based` is set, which is true for every `-F` recording regardless of event (`converter.rs`, `off_cpu_weight_per_sample`; `event_interpretation.rs`).
+* `Other event` markers carry only `type` and `cause.stack`; their schema has no fields (`samply/src/shared/process_sample_data.rs`, `OtherEventMarker`).
+* With `-F`, each sample record carries `PERIOD`; with `-c`, the sample type omits `PERIOD` and the attribute's `sample_period` gives the fixed value (`perf evlist -v`).
+* With `-F cycles`, `threadCPUDelta` holds cycle counts written as nanoseconds, because the converter treats every period as time (`// TODO: Detect event type`).
+* The Firefox Profiler's `ProfileMeta.extra` (`src/types/profile.ts`, `ExtraProfileInfoSection`) holds labeled sections for the profile info panel. The profile upgraders never touch it, and fxprof-processed-profile does not expose it.
+* The format's `samples.weight` is a JSON number array, while fxprof-processed-profile takes `weight: i32` in `Thread::add_sample`.
+
+## Goals
+
+* samply writes each `Other event` marker's period.
+* samply names every perf event, its sampling mode, and the sample weight mode in `meta.extra`.
+* `samply import --weight-by-period` writes the main event's period as the sample weight.
+* samply stops writing non-clock periods into `threadCPUDelta`.
+* pollard weights shares by period when the profile carries periods, and reports the main event's name.
+
+## Non-goals
+
+* Event selection for `samply record` (`-e`, `-c`): `perf record` already covers it, and `samply import` converts the result.
+* Widening fxprof-processed-profile's `weight` type from `i32`.
+* A new `WeightType` variant; the Firefox Profiler accepts only `samples`, `tracing-ms`, and `bytes`.
+  With `--weight-by-period`, the Firefox Profiler therefore labels weighted totals as samples.
+* Making `sampling_is_time_based` event-aware. `-F cycles` keeps its time-based profile interval.
+* CPU deltas for `-c cpu-clock` imports, whose records carry no period. They stay 0, as today.
+* Recovering periods of duplicate-timestamp samples, which the converter already drops.
+
+## samply changes
+
+The samply work splits into two independent pull requests against `mstange/samply`, so the maintainer can take the bug fix without the feature.
+
+### Pull request 1: periods and event metadata
+
+**fxprof-processed-profile** gains `Profile::add_extra_info_section(label, entries)`, written in `write_meta_json` to `meta.extra` as the Firefox Profiler defines it.
+Each entry has a label, a format, and a value, and samply uses only the `string` format.
+The Firefox Profiler formats these values without a string table, so string-index formats such as `unique-string` must not be used.
+The method is additive, so the crate's public API stays compatible.
+
+**Event metadata.** `samply import` adds one `meta.extra` section labeled `Perf events`.
+It holds one `string` entry per perf event attribute in attribute order, followed by one entry labeled `Sample weight`.
+An event entry's label is the event name exactly as `EventInterpretation::event_names` holds it, which includes modifiers and PMU prefixes such as `cycles:u` or `cpu_core/cycles/`, and placeholders such as `<unknown event 2>`.
+The values follow this grammar, with `N` a decimal integer:
+
+```
+event value  = "frequency " N " Hz" | "period " N
+weight value = "period" | "1"
+```
+
+`Sample weight` reads `period` when `--weight-by-period` was passed and `1` otherwise.
+An attribute that does not sample, such as a member of a leader-sampled group, gets the value `no sampling`, which is outside the grammar, so pollard ignores it.
+The first event entry is the main event, which becomes the samples track, because `EventInterpretation::main_event_attr_index` is always 0.
+pollard reads this section to name the samples track, to find fixed periods, and to tell weighted samples apart. The Firefox Profiler shows it in the profile info panel, where duplicate labels only cause a React key warning.
+
+**Marker period.** `OtherEventMarker` gains an integer field `period`.
+The value is the sample record's `period` when present, and the attribute's fixed `sample_period` otherwise, so `-c` recordings carry a period too.
+`EventInterpretation` gains `fixed_periods: Vec<Option<u64>>`, one per attribute, filled from each attribute's `SamplingPolicy::Period`.
+`samply record` builds its `EventInterpretation` by hand in `linux/profiler.rs` and sets `fixed_periods: vec![None]`.
+Marker number fields are `f64`, which represents periods exactly up to 2^53, and the writer prints whole values without a fraction.
+This field is on by default because it is additive, and the Firefox Profiler shows it in the marker tooltip.
+It changes every import's marker schema, so the pull request description says so and offers to put it behind the flag.
+
+**Sample weight.** `samply import --weight-by-period` plumbs through `ProfileCreationProps` as `weight_by_period: bool`.
+With the flag, all three `add_sample` calls in `handle_main_event_sample` pass the main event's period as the sample weight instead of 1, from the record or the fixed period.
+Periods above `i32::MAX` saturate to `i32::MAX`, and samply prints one warning per import naming the event and the largest period seen.
+The flag also sets the off-CPU weight per sample to 0, because off-CPU time produces no events of the main event and a weight of 1 would mix units with event counts.
+`modify_last_sample`, whose `+=` could overflow, is only called from the macOS recorder and not on the import path.
+Without the flag, output is unchanged apart from the marker field and `meta.extra`.
+`samply record` never passes the flag.
+
+The weight decision lives in one pure function so it can be unit tested without building a `Converter`:
+
+```rust
+/// Weight of one main-event sample, and whether it saturated.
+fn sample_weight(record_period: Option<u64>, fixed_period: Option<u64>, weight_by_period: bool) -> (i32, bool)
+```
+
+It returns `(1, false)` when the flag is off, and also when the flag is on but neither period is known.
+The derivation of `fixed_periods` from the attributes' sampling policies is a second pure function with its own unit tests.
+
+### Pull request 2: CPU delta only for clock events
+
+`EventInterpretation` gains `main_event_is_clock: bool`, true when the main attribute is the software `cpu-clock` or `task-clock` event, as `divine_from_attrs` already matches for `sampling_is_time_based`.
+`samply record`'s hand-built `EventInterpretation` in `linux/profiler.rs` sets it to `false`, which changes nothing, because the record path always has context-switch data and never reaches the period branch.
+`handle_main_event_sample` converts the record's period to `CpuDelta` only when `main_event_is_clock` is true.
+For other main events without context-switch data, the CPU delta becomes 0 instead of an event count.
+This changes the Firefox Profiler's CPU graph for such imports from wrong values to no CPU data, and the pull request says so.
+The pull request covers the CPU delta only, does not depend on pull request 1, and leaves the profile interval alone.
+
+## pollard changes
+
+**Perf event metadata.** `RawMeta` gains `extra: Vec<RawExtraSection>` (`#[serde(default)]`), where a section has a `label` and `entries` of `{label, format, value}`.
+`Profile::perf_events()` parses the `Perf events` section into the event list and the sample weight mode, and ignores values that do not match the grammar.
+`list_events` and marker weight lookup both use it.
+
+**Weight per item.** `Profile` gains `weighted_stack_indices`, which yields `(Option<usize>, u64)` pairs of stack index and weight.
+The existing `stack_indices` becomes a wrapper that drops the weight, so callers move over one at a time.
+A sample's weight is `samples.weight[i]` when the `Perf events` section says `Sample weight: period`, and 1 otherwise.
+A marker's weight is its `data.period` when present, then the fixed period of that event from the `Perf events` section, then 1. samply always writes `period`, so the fallbacks exist for older samply output and hand-built fixtures.
+`RawMarkerData` gains `period: Option<f64>`.
+
+**Weighted tools.** These tools add weights instead of counting items: `top_functions`, `call_tree`, `stacks_containing`, `folded_stacks`, `top_groups`, `compare_profiles`, `compare_functions`, `source_for_function`, `asm_for_function`, and the rankings in `summary` (`top_modules`, `top_self_functions`, `top_total_functions`).
+Their totals and percentages therefore mean event counts, e.g. cache misses, rather than sample counts.
+`source_for_function` and `asm_for_function` iterate `samples.stack` directly and keep doing so, reading the sample weight for each sample, so their event and time-range behavior does not change.
+`summary.total_samples`, per-thread sample counts, `describe_profile`, `view_stats`, and `list_events` keep reporting raw sample and marker counts, because they describe the recording rather than rank code.
+`summary`'s `function_recurs_in_any_stack` and `view_stats` stay permanent callers of the unweighted `stack_indices`.
+
+**Accumulators.** `Counts`, shared by `top_functions`, `compare_profiles`, and `top_groups`, becomes `{ samples: u64, weight: u64 }` for self and total.
+`*_samples` and `*_pct` report `weight`, and `compare_profiles`' `*_ms` columns keep using `samples` times the interval, because an event count has no time unit.
+`call_tree`'s `AggNode` totals, `stacks_containing`'s per-stack and matched-frame counts, and `folded_stacks`' per-line counts accumulate weight in their existing `u64` fields, whose names stay unchanged.
+For profiles without weight information every weight is 1, so every output is identical to today's.
+
+**`weighted` flag.** Outputs of the weighted tools gain `weighted: bool`, including `summary` and `FoldedStacksOutput`, since the folded text has no slot for it.
+For the samples track it is true when the `Perf events` section says `Sample weight: period`.
+For a marker event it is true when at least one selected marker of that event has a `period` or its event has a fixed period.
+It does not depend on the weight values, so a `-c 1` recording is still weighted.
+`compare_profiles` reports `weighted_a` and `weighted_b`.
+When they differ it adds a `note` saying that one side's percentages are event shares and the other's are sample shares, which are not directly comparable.
+
+**Event names.** `list_events` from sub-project A reads the `Perf events` section.
+The samples entry keeps `name: "samples"` and gains `event` (the raw label of the first entry, e.g. `cycles:u`) and `sampling` (e.g. `frequency 999 Hz`) when the section exists.
+Marker entries gain `sampling` from the entry with the same label.
+When several entries share a label, the first one wins, because markers of equally named events are indistinguishable anyway.
+
+**Docs.** The `profile-recording` skill drops the advice to prefer `-c`, and says to pass `--weight-by-period` to `samply import`.
+
+## Errors
+
+* A marker `period` that is 0, negative, or not finite is ignored, and the lookup falls back to the event's fixed period, then 1. samply writes 0 when neither the record nor the attribute has a period, because the field cannot be omitted.
+* A `Perf events` value that does not match the grammar is ignored.
+* A `samples.weight` entry that is negative or not finite weighs 0.
+
+## Testing
+
+* samply: unit tests for `sample_weight` cover a record period, a fixed period only, neither with the flag on (`(1, false)`), the flag off, and saturation above `i32::MAX`.
+* samply: unit tests for the `fixed_periods` derivation cover frequency and period attributes.
+* samply: the fixtures from sub-project A's `regenerate.sh` are re-imported with and without the flag.
+  The default import differs from before only by the marker field and the `meta.extra` section.
+  In the flagged import, the per-tid sums of sample weights on non-CPU threads equal the per-tid sums of `perf script -F tid,period` for the main event, over the samples samply keeps (tid 0 and duplicate timestamps are dropped).
+* samply: a `-F cycles` recording with context switches, imported with the flag, has off-CPU samples of weight 0.
+* samply: a unit test for `main_event_is_clock` covers `cpu-clock`, `task-clock`, and `cycles`, and a `-F cycles` import without context switches has CPU deltas of 0.
+* pollard: unit tests cover `perf_events()` parsing, including invalid values, and weight lookup for samples with and without `Sample weight: period`, markers with `period`, markers with only a fixed period, and neither.
+* pollard: aggregation tests use a fixture where two functions have equal counts but different periods, and check that shares follow periods in each weighted tool, `weighted` is true, and `compare_profiles` `_ms` columns follow counts.
+* pollard: a `compare_profiles` test with one weighted and one unweighted side checks `weighted_a`, `weighted_b`, and the `note`.
+* pollard: all existing tests pass unchanged, because their fixtures carry no weight information.
+
+## Rollout
+
+samply pull request 1 lands on branch `import-period`, and pull request 2 on branch `cpu-delta-clock-events`, both from `upstream/main` in the samply fork and independent of each other.
+pollard's part lands on branch `period-weighting`, stacked on `shared-tables-format`.
+pollard does not depend on the samply pull requests being merged: it reads the new fields when present and behaves as today otherwise.

skills/profile-recording/SKILL.md

@@ -127,18 +127,24 @@ surface available — `top_functions`, `call_tree`, `summary`,
 
 ## Recording other perf events
 
-`samply record` samples cycles only. For other events, record with `perf` and import:
+`samply record` samples cycles only. For other events, record with `perf` and import.
+
+`--weight-by-period` needs a samply build that includes period support, which is not yet in a samply release. samply 0.13.1 and earlier reject the flag.
 
 ```sh
 perf record -e cycles,cache-misses,instructions -g -- <cmd>
-samply import perf.data --save-only -o /tmp/profile.json.gz
+samply import perf.data --weight-by-period --save-only -o /tmp/profile.json.gz
 ```
 
 The first `-e` event becomes the samples track, which pollard queries by default.
 Every other event becomes markers named after it; pass that name as `event`, e.g. `event="cache-misses"`.
 `describe_profile` lists the events a profile contains.
 
-Use a fixed period (`-c N`) instead of a frequency (`-F`) when comparing counts across events.
-In frequency mode perf varies the period per sample, and pollard counts samples without weighting them by period.
+Pass `--weight-by-period` to `samply import` so the samples track counts events instead of samples.
+samply also records each marker's period, so pollard weights every event by its period and reports `weighted: true` in tool outputs.
+This makes frequency mode (`-F`) and fixed periods (`-c N`) equally usable.
+Without it, pollard counts samples of the first event, which in frequency mode biases shares toward code that ran while the period was small.
+A fixed period (`-c N`) does not save you from this: pollard always weights marker events by their period, so without `--weight-by-period` the samples track counts raw samples while marker totals are already scaled by N, and the two disagree by that factor. Pass `--weight-by-period` whenever you compare the samples track against another event.
+`describe_profile` names the first event and how each event was sampled.
 
 samply saves `.jslb.gz` by default; pollard loads those too.

src/profile/mod.rs

@@ -12,6 +12,7 @@ pub mod event_source;
 pub(crate) mod jslb;
 pub mod load;
 pub mod parsed;
+pub mod perf_events;
 pub mod raw;
 pub mod symbolicate;
 pub mod tables;