Perf#269
Open
MatthewKhouzam wants to merge 2 commits into
Open
Conversation
Add a new trace type that parses Linux perf.data files directly, following the layout described in perf_data_format.md. New bundle: org.eclipse.tracecompass.incubator.perf.core - PerfDataReader: random-access reader for the file header (PERFILE2 / PERFILE1, little- and big-endian), attrs section (perf_event_attr + per-attr event IDs), feature sections (hostname, osrelease, arch, cpudesc, cpuid, cmdline, nr_cpus), and data-stream records (MMAP, MMAP2, COMM, FORK, EXIT, SAMPLE, SWITCH, SWITCH_CPU_WIDE, LOST, THROTTLE, UNTHROTTLE, FINISHED_ROUND, FINISHED_INIT), including the trailing sample_id block when sample_id_all is set. Piped headers are detected but their record stream is not decoded yet. - PerfDataTrace: TmfTrace subclass registered via plugin.xml, with aspects for timestamp, event type, PID, TID, CPU, IP, Address, Comm and Filename. Exposes trace metadata through ITmfPropertiesProvider. New bundle: org.eclipse.tracecompass.incubator.perf.core.tests - PerfDataTraceTest with validate/reader/event-iteration tests, driven by a bundled sample perf.data file. Register both bundles in tracetypes/pom.xml. This code was written with the assistance of claude opus 4.7 Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com> Change-Id: I43f44e9f1eaef3c886c36d9eb3a4565c92bc96bc
📝 WalkthroughWalkthroughA new Eclipse Trace Compass plug-in is introduced for parsing and analyzing Linux perf.data trace files. The implementation spans two bundles—core parsing logic and test suite—with modules for file reading, callchain sampling analysis, and memory-mapped symbol resolution using state systems. Changes
Sequence DiagramsequenceDiagram
participant User
participant TMF as TMF/Trace
participant PerfDataTrace as PerfDataTrace
participant Reader as PerfDataReader
participant Analysis as AnalysisModule
participant StateSystem as StateSystem
participant SymbolProvider as SymbolProvider
User->>TMF: Open perf.data file
TMF->>PerfDataTrace: initTrace()
PerfDataTrace->>Reader: new PerfDataReader(file)
Reader->>Reader: Parse header & attrs
Note over Reader: Extract magic, features,<br/>metadata, data offset
User->>TMF: Iterate events
TMF->>PerfDataTrace: parseEvent(context)
PerfDataTrace->>Reader: readRecordAt(offset)
Reader->>Reader: Decode record type<br/>(MMAP2, SAMPLE, etc.)
Reader-->>PerfDataTrace: PerfRecord
PerfDataTrace->>PerfDataTrace: Convert to TmfEvent
PerfDataTrace-->>TMF: TmfEvent
TMF->>Analysis: Process event
alt SAMPLE record
Analysis->>Analysis: Extract callchain
Analysis->>Analysis: Build call stack
Analysis-->>TMF: CallStackElement
else MMAP/MMAP2 record
Analysis->>StateSystem: Write load_base
StateSystem-->>Analysis: State updated
end
User->>SymbolProvider: Resolve address<br/>(pid, timestamp, addr)
SymbolProvider->>StateSystem: Query MMAP quark
StateSystem-->>SymbolProvider: filename@timestamp
SymbolProvider->>SymbolProvider: Load ELF & lookup symbol
SymbolProvider-->>User: TmfLibrarySymbol
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (2 warnings)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (7)
tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapSymbolProvider.java (1)
48-48: ⚖️ Poor tradeoffSymbol mapping cache grows unbounded.
fSymbolMappingcachesIMappingFileinstances per filename but is never cleared. For long traces with many unique libraries, this could consume significant memory. Consider implementing a size-bounded cache or clearing on dispose.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapSymbolProvider.java` at line 48, The fSymbolMapping Map in PerfDataMmapSymbolProvider stores IMappingFile instances indefinitely causing unbounded growth; fix this by making the cache size-bounded or ensuring it is cleared on lifecycle end—either replace the HashMap with an LRU-capable map (e.g., LinkedHashMap with accessOrder and max size eviction) keyed by filename or explicitly clear fSymbolMapping in the provider's dispose/close method (add/override dispose() in PerfDataMmapSymbolProvider to iterate/close any IMappingFile resources if needed and call fSymbolMapping.clear()). Ensure any IMappingFile cleanup is performed before removing entries.tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataTraceTest.java (2)
129-135: 💤 Low valuePotential
ClassCastExceptionon field access.The cast
(String) rec.getField("filename")on line 131 assumes the field is always aString. If the reader ever returns a different type (or the record is malformed), this will throw aClassCastException. Since line 129 already asserts non-null, consider usinginstanceofbefore casting for robustness.Suggested safer cast
assertNotNull("filename", rec.getField("filename")); //$NON-NLS-1$ //$NON-NLS-2$ - assertTrue("filename non-empty", //$NON-NLS-1$ - !((String) rec.getField("filename")).isEmpty()); //$NON-NLS-1$ + Object fnObj = rec.getField("filename"); //$NON-NLS-1$ + assertTrue("filename is a String", fnObj instanceof String); //$NON-NLS-1$ + assertTrue("filename non-empty", !((String) fnObj).isEmpty()); //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataTraceTest.java` around lines 129 - 135, The test assumes rec.getField("filename") is a String and casts directly, which can throw ClassCastException; change the assertion to first verify the field is a non-null String (e.g. use instanceof to assert rec.getField("filename") instanceof String) and only then test that ((String) rec.getField("filename")).isEmpty() (or use String.valueOf(rec.getField("filename")).isEmpty() if you intend to accept non-String values), referencing the existing rec.getField("filename") usage and the surrounding assertions so the filename check is robust against non-String/malformed records.
198-208: 💤 Low valueFallback path returned without existence check.
resolvePath()returns the last fallback path unconditionally (line 207) even if the file doesn't exist. This may cause tests to fail with confusing "file not found" errors rather than a clear message about the missing test fixture.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataTraceTest.java` around lines 198 - 208, resolvePath currently returns the final fallback File unconditionally even if it doesn't exist; update resolvePath to verify the existence of that final candidate (and the earlier candidates) and if none exist throw an informative exception (e.g., IllegalStateException) that lists the attempted paths (direct, bundleRel, and the fallback) so tests fail with a clear message rather than returning a non-existent File; reference the resolvePath method and the local variables direct and bundleRel when locating where to add the existence check and the exception.tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/PerfDataCallchainAnalysisModule.java (2)
296-301: ⚖️ Poor tradeoffLinear merge loop in sampling request.
The merge logic iterates through
fSiteslinearly for each sample. For traces with many distinct callsites, this O(n²) behavior could be slow. AMap<Object, AggregatedCallSite>keyed bysample.getObject()would provide O(1) lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/PerfDataCallchainAnalysisModule.java` around lines 296 - 301, The current O(n²) merge loop using fSites should be replaced with a Map keyed by sample.getObject() to give O(1) lookups: change the collection type from the current List/Collection fSites to a Map<Object, AggregatedCallSite> (or add a parallel Map) in PerfDataCallchainAnalysisModule, populate the map from existing fSites when initializing, then in the insertion logic replace the linear loop that checks existing.getObject().equals(sample.getObject()) and calls existing.merge(sample) with a direct map lookup by sample.getObject() and call merge if present or put a new AggregatedCallSite otherwise; also update any other places that iterate fSites to either iterate map.values() or keep a single source of truth to avoid inconsistencies.
170-203: ⚖️ Poor tradeoffLinear lookup on every event may impact performance.
getElement()performs a linear stream search throughgetRootElements()for each event (line 175-177), and another linear search through children (line 194-196). For traces with many processes/threads and millions of samples, this O(n) lookup per event could become a bottleneck.Consider caching elements in a
Map<Integer, ICallStackElement>keyed by pid (and a nested map for tid) to achieve O(1) lookups.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/PerfDataCallchainAnalysisModule.java` around lines 170 - 203, getElement currently scans getRootElements() and processEl.getChildrenElements() on every event (using streams) which is O(n) per event; replace that linear lookup with cached maps: add a Map<Integer, ICallStackElement> pidToProcess and for each process a Map<Integer, ICallStackElement> pidToTidMap (or a Map<Integer, Map<Integer, ICallStackElement>>) keyed by pid→tid to provide O(1) lookups inside getElement; when you create a new processEl or threadEl (the CallStackElement instances constructed in getElement) insert them into the maps, and when adding/removing root elements update the maps accordingly; ensure getElement consults the maps first instead of calling getRootElements() or iterating children to preserve existing behavior.tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataCallchainTest.java (1)
78-80: 💤 Low valueRedundant assertion placement.
assertNotNull(file)on line 79 is placed afterfilehas already been used multiple times (lines 49-50, 57, etc.). If the file were null, the test would have already failed. Consider moving this assertion to the beginning or removing it.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataCallchainTest.java` around lines 78 - 80, The assertNotNull(file) is redundant where it's placed after file has already been dereferenced; either remove this late check or move it up immediately after the code that constructs/obtains the file variable so the null check happens before any use. Update the PerfDataCallchainTest test to either delete the trailing assertNotNull(file) or relocate it to directly follow the assignment/creation of file (the variable named file used earlier in the test) so that any null-file failure occurs before calls that use file and before assertions on samples.tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java (1)
684-695: 💤 Low valuePotential integer overflow in callchain size validation.
Line 686 compares
nr(along) againstbody.remaining() / 8, but ifnris extremely large (close toLong.MAX_VALUE), the subsequent cast tointon line 690 could overflow silently. While unlikely with valid perf data, consider clamping toInteger.MAX_VALUE.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java` around lines 684 - 695, The callchain parsing in PerfDataReader reads nr as a long and later casts to int without guarding against very large values; update the validation to ensure nr is non-negative and <= Integer.MAX_VALUE and also <= body.remaining()/8 before allocating the array and casting, e.g., clamp or fail when nr > Integer.MAX_VALUE (and treat it as malformed), then safely cast (int)nr for the new long[nr] and the loop that fills ips before putting "callchain" into fields.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/about.html`:
- Line 19: Replace the plaintext HTTP links in the about page content with
HTTPS: locate the occurrences of "http://www.eclipse.org/legal/epl-2.0" and the
other external link using "http://" in the about.html content and change them to
"https://..." so both external links use HTTPS to avoid mixed/insecure
navigation from plugin metadata pages; ensure you update both occurrences (the
one shown and the other at the second occurrence).
In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/about.html`:
- Line 19: Update the anchor hrefs in the about.html anchors that currently use
"http://www.eclipse.org/..." (e.g., the license link
"http://www.eclipse.org/legal/epl-2.0" and the other http anchor on the page) to
use HTTPS ("https://...") so both external links use secure https URLs; edit the
anchor href attributes in the about.html markup accordingly.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`:
- Around line 81-86: In PerfDataReader (inside the fHeader.isPiped() branch)
remove the dead standalone calls to Map.of() and
Collections.unmodifiableMap(byId) (they produce values that are never assigned);
either delete these statements or replace them by assigning their results to the
appropriate fields (e.g. assign Map.of() to fFeatures or fMetadata if that was
intended) so only meaningful assignments remain for fAttrs, fDefaultAttr,
fFeatures, and fMetadata.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfFileHeader.java`:
- Around line 135-137: getFeatures() currently returns the internal fFeatures
array from the immutable PerfFileHeader class which allows external mutation;
change getFeatures() to return a defensive copy of fFeatures (e.g., create and
return a new long[] copy or use Arrays.copyOf) so callers cannot modify the
internal state while keeping the method signature the same.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.java`:
- Around line 79-81: The current early return for pid < 0 drops kernel/module
mappings (pid == -1); instead, change the pid check so pid == -1 is NOT
discarded and is stored in a dedicated global bucket inside
PerfDataMmapStateProvider (create or reuse a globalMmaps/map for kernel
mappings) while preserving ignoring other negative pids if necessary, and update
the symbol lookup in PerfDataMmapSymbolProvider to consult that global bucket
when a per-process lookup misses; modify the mmap-handling branch where the pid
check occurs to branch on pid == -1 (store to global) vs pid >= 0 (store
per-process) and ensure lookup code queries globalMmaps as a fallback.
- Around line 56-60: The eventHandle currently filters to only MMAP/MMAP2 and
ignores PERF_RECORD_FORK and PERF_RECORD_EXIT which breaks the mmap state across
forks/exits; update eventHandle to also handle events whose type corresponds to
PERF_RECORD_FORK and PERF_RECORD_EXIT (as emitted by PerfDataReader.decodeBody),
extract parent and child PIDs from the event payload for FORK and the exiting
PID for EXIT, and on FORK copy the parent's PID subtree/mappings into the child
PID subtree in the state system, while on EXIT clear the exiting PID's
subtree/mappings so stale mappings are removed; keep existing MMAP/MMAP2
handling intact and use the same state-system API you already use for writing
mapping entries to perform the copy and clear operations.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfDataTrace.java`:
- Around line 224-229: In seekEvent(double ratio) the multiplication is done
after casting the clamped ratio to long, so (long) Math.max(...) becomes 0 for
ratios <1; change the calculation in PerfDataTrace.seekEvent so you multiply the
clamped double ratio by span first and only then cast to long (e.g. compute
(long)(clampedRatio * span) or use DoubleMath) to produce the correct offset
passed to new TmfLongLocation(offset).
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfEventType.java`:
- Around line 28-29: The static TYPES map in PerfEventType is currently a
HashMap and lookup() does an unsynchronized read-then-write, causing concurrent
mutation risk; change TYPES to a ConcurrentHashMap and update lookup() to use
TYPES.computeIfAbsent(key, k -> /* create new ITmfEventType for k */) so unknown
event types are safely cached atomically; reference the PerfEventType class, the
TYPES field, and the lookup() method when making these changes.
---
Nitpick comments:
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataCallchainTest.java`:
- Around line 78-80: The assertNotNull(file) is redundant where it's placed
after file has already been dereferenced; either remove this late check or move
it up immediately after the code that constructs/obtains the file variable so
the null check happens before any use. Update the PerfDataCallchainTest test to
either delete the trailing assertNotNull(file) or relocate it to directly follow
the assignment/creation of file (the variable named file used earlier in the
test) so that any null-file failure occurs before calls that use file and before
assertions on samples.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataTraceTest.java`:
- Around line 129-135: The test assumes rec.getField("filename") is a String and
casts directly, which can throw ClassCastException; change the assertion to
first verify the field is a non-null String (e.g. use instanceof to assert
rec.getField("filename") instanceof String) and only then test that ((String)
rec.getField("filename")).isEmpty() (or use
String.valueOf(rec.getField("filename")).isEmpty() if you intend to accept
non-String values), referencing the existing rec.getField("filename") usage and
the surrounding assertions so the filename check is robust against
non-String/malformed records.
- Around line 198-208: resolvePath currently returns the final fallback File
unconditionally even if it doesn't exist; update resolvePath to verify the
existence of that final candidate (and the earlier candidates) and if none exist
throw an informative exception (e.g., IllegalStateException) that lists the
attempted paths (direct, bundleRel, and the fallback) so tests fail with a clear
message rather than returning a non-existent File; reference the resolvePath
method and the local variables direct and bundleRel when locating where to add
the existence check and the exception.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/PerfDataCallchainAnalysisModule.java`:
- Around line 296-301: The current O(n²) merge loop using fSites should be
replaced with a Map keyed by sample.getObject() to give O(1) lookups: change the
collection type from the current List/Collection fSites to a Map<Object,
AggregatedCallSite> (or add a parallel Map) in PerfDataCallchainAnalysisModule,
populate the map from existing fSites when initializing, then in the insertion
logic replace the linear loop that checks
existing.getObject().equals(sample.getObject()) and calls existing.merge(sample)
with a direct map lookup by sample.getObject() and call merge if present or put
a new AggregatedCallSite otherwise; also update any other places that iterate
fSites to either iterate map.values() or keep a single source of truth to avoid
inconsistencies.
- Around line 170-203: getElement currently scans getRootElements() and
processEl.getChildrenElements() on every event (using streams) which is O(n) per
event; replace that linear lookup with cached maps: add a Map<Integer,
ICallStackElement> pidToProcess and for each process a Map<Integer,
ICallStackElement> pidToTidMap (or a Map<Integer, Map<Integer,
ICallStackElement>>) keyed by pid→tid to provide O(1) lookups inside getElement;
when you create a new processEl or threadEl (the CallStackElement instances
constructed in getElement) insert them into the maps, and when adding/removing
root elements update the maps accordingly; ensure getElement consults the maps
first instead of calling getRootElements() or iterating children to preserve
existing behavior.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`:
- Around line 684-695: The callchain parsing in PerfDataReader reads nr as a
long and later casts to int without guarding against very large values; update
the validation to ensure nr is non-negative and <= Integer.MAX_VALUE and also <=
body.remaining()/8 before allocating the array and casting, e.g., clamp or fail
when nr > Integer.MAX_VALUE (and treat it as malformed), then safely cast
(int)nr for the new long[nr] and the loop that fills ips before putting
"callchain" into fields.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapSymbolProvider.java`:
- Line 48: The fSymbolMapping Map in PerfDataMmapSymbolProvider stores
IMappingFile instances indefinitely causing unbounded growth; fix this by making
the cache size-bounded or ensuring it is cleared on lifecycle end—either replace
the HashMap with an LRU-capable map (e.g., LinkedHashMap with accessOrder and
max size eviction) keyed by filename or explicitly clear fSymbolMapping in the
provider's dispose/close method (add/override dispose() in
PerfDataMmapSymbolProvider to iterate/close any IMappingFile resources if needed
and call fSymbolMapping.clear()). Ensure any IMappingFile cleanup is performed
before removing entries.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 4ce559c7-f8b7-47a5-948c-7baf52bfd7f3
📒 Files selected for processing (50)
tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.classpathtracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.gitignoretracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.projecttracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.core.resources.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.core.runtime.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.jdt.core.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.jdt.ui.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.pde.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/META-INF/MANIFEST.MFtracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/about.htmltracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/build.propertiestracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/plugin.propertiestracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataCallchainTest.javatracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataTraceTest.javatracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/traces/perf-callgraph.datatracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/traces/perf.datatracetypes/org.eclipse.tracecompass.incubator.perf.core/.classpathtracetypes/org.eclipse.tracecompass.incubator.perf.core/.gitignoretracetypes/org.eclipse.tracecompass.incubator.perf.core/.projecttracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.core.resources.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.core.runtime.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.jdt.core.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.jdt.ui.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.pde.api.tools.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.pde.prefstracetypes/org.eclipse.tracecompass.incubator.perf.core/META-INF/MANIFEST.MFtracetypes/org.eclipse.tracecompass.incubator.perf.core/about.htmltracetypes/org.eclipse.tracecompass.incubator.perf.core/build.propertiestracetypes/org.eclipse.tracecompass.incubator.perf.core/plugin.propertiestracetypes/org.eclipse.tracecompass.incubator.perf.core/plugin.xmltracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/Activator.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfConstants.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfEventAttr.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfFileHeader.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfFileSection.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfRecord.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/PerfDataCallchainAnalysisModule.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/package-info.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/package-info.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapAnalysisModule.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapSymbolProvider.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapSymbolProviderFactory.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/TmfLibrarySymbol.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/package-info.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfDataTrace.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfEventType.javatracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/package-info.javatracetypes/pom.xml
| ("Content"). Unless otherwise indicated below, the Content | ||
| is provided to you under the terms and conditions of the Eclipse | ||
| Public License Version 2.0 ("EPL"). A copy of the EPL is | ||
| available at <a href="http://www.eclipse.org/legal/epl-2.0">http://www.eclipse.org/legal/epl-2.0</a>. |
There was a problem hiding this comment.
Use HTTPS links in About page content.
Both external links are plaintext HTTP; switch them to HTTPS to avoid mixed/insecure navigation from plugin metadata pages.
Suggested fix
- available at <a href="http://www.eclipse.org/legal/epl-2.0">http://www.eclipse.org/legal/epl-2.0</a>.
+ available at <a href="https://www.eclipse.org/legal/epl-2.0">https://www.eclipse.org/legal/epl-2.0</a>.
@@
- code in the Content and such source code may be obtained at <a
- href="http://www.eclipse.org/">http://www.eclipse.org</a>.
+ code in the Content and such source code may be obtained at <a
+ href="https://www.eclipse.org/">https://www.eclipse.org</a>.Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/about.html` at
line 19, Replace the plaintext HTTP links in the about page content with HTTPS:
locate the occurrences of "http://www.eclipse.org/legal/epl-2.0" and the other
external link using "http://" in the about.html content and change them to
"https://..." so both external links use HTTPS to avoid mixed/insecure
navigation from plugin metadata pages; ensure you update both occurrences (the
one shown and the other at the second occurrence).
| ("Content"). Unless otherwise indicated below, the Content | ||
| is provided to you under the terms and conditions of the Eclipse | ||
| Public License Version 2.0 ("EPL"). A copy of the EPL is | ||
| available at <a href="http://www.eclipse.org/legal/epl-2.0">http://www.eclipse.org/legal/epl-2.0</a>. |
There was a problem hiding this comment.
Use HTTPS for external license/project links.
Both anchors currently point to plaintext HTTP URLs. Switch them to HTTPS to avoid mixed/insecure link targets in distributed plugin metadata pages.
Suggested patch
- available at <a href="http://www.eclipse.org/legal/epl-2.0">http://www.eclipse.org/legal/epl-2.0</a>.
+ available at <a href="https://www.eclipse.org/legal/epl-2.0">https://www.eclipse.org/legal/epl-2.0</a>.
@@
- code in the Content and such source code may be obtained at <a
- href="http://www.eclipse.org/">http://www.eclipse.org</a>.
+ code in the Content and such source code may be obtained at <a
+ href="https://www.eclipse.org/">https://www.eclipse.org</a>.Also applies to: 32-32
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/about.html` at line
19, Update the anchor hrefs in the about.html anchors that currently use
"http://www.eclipse.org/..." (e.g., the license link
"http://www.eclipse.org/legal/epl-2.0" and the other http anchor on the page) to
use HTTPS ("https://...") so both external links use secure https URLs; edit the
anchor href attributes in the about.html markup accordingly.
Comment on lines
+81
to
+86
| if (fHeader.isPiped()) { | ||
| fAttrs = List.of(); | ||
| Map.of(); | ||
| fDefaultAttr = null; | ||
| fFeatures = Map.of(); | ||
| fMetadata = Map.of(); |
There was a problem hiding this comment.
Dead code statements - results not assigned.
Lines 83 and 95 call Map.of() and Collections.unmodifiableMap(byId) respectively, but their return values are not assigned to any variable. These statements have no effect.
Proposed fix - remove dead statements
if (fHeader.isPiped()) {
fAttrs = List.of();
- Map.of();
fDefaultAttr = null;
fFeatures = Map.of();
fMetadata = Map.of();
} else {
fAttrs = parseAttrs(fHeader);
Map<Long, PerfEventAttr> byId = new LinkedHashMap<>();
for (PerfEventAttr attr : fAttrs) {
for (Long id : attr.getIds()) {
byId.put(id, attr);
}
}
- Collections.unmodifiableMap(byId);
+ // byId is constructed but not currently used; consider removing if not needed
fDefaultAttr = fAttrs.isEmpty() ? null : fAttrs.get(0);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`
around lines 81 - 86, In PerfDataReader (inside the fHeader.isPiped() branch)
remove the dead standalone calls to Map.of() and
Collections.unmodifiableMap(byId) (they produce values that are never assigned);
either delete these statements or replace them by assigning their results to the
appropriate fields (e.g. assign Map.of() to fFeatures or fMetadata if that was
intended) so only meaningful assignments remain for fAttrs, fDefaultAttr,
fFeatures, and fMetadata.
Comment on lines
+135
to
+137
| public long[] getFeatures() { | ||
| return fFeatures; | ||
| } |
There was a problem hiding this comment.
Internal array exposed breaks immutability.
getFeatures() returns the internal fFeatures array directly, allowing callers to modify its contents. Since PerfFileHeader is documented as immutable (final class, private fields), this is a contract violation.
Proposed fix - return a defensive copy
public long[] getFeatures() {
- return fFeatures;
+ return fFeatures.clone();
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| public long[] getFeatures() { | |
| return fFeatures; | |
| } | |
| public long[] getFeatures() { | |
| return fFeatures.clone(); | |
| } |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfFileHeader.java`
around lines 135 - 137, getFeatures() currently returns the internal fFeatures
array from the immutable PerfFileHeader class which allows external mutation;
change getFeatures() to return a defensive copy of fFeatures (e.g., create and
return a new long[] copy or use Arrays.copyOf) so callers cannot modify the
internal state while keeping the method signature the same.
Comment on lines
+56
to
+60
| protected void eventHandle(ITmfEvent event) { | ||
| String name = event.getType().getName(); | ||
| if (!MMAP_NAME.equals(name) && !MMAP2_NAME.equals(name)) { | ||
| return; | ||
| } |
There was a problem hiding this comment.
Handle FORK/EXIT here as part of the mmap state model.
Filtering everything except MMAP/MMAP2 makes the state system wrong across process lifetime changes: forked children never inherit the parent's mappings, and exited PIDs keep stale mappings forever until the trace ends. Since PerfDataReader.decodeBody(...) already emits PERF_RECORD_FORK and PERF_RECORD_EXIT, this provider should copy the parent's subtree at fork time and clear the PID subtree on exit to avoid bad symbol resolution after fork or PID reuse.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.java`
around lines 56 - 60, The eventHandle currently filters to only MMAP/MMAP2 and
ignores PERF_RECORD_FORK and PERF_RECORD_EXIT which breaks the mmap state across
forks/exits; update eventHandle to also handle events whose type corresponds to
PERF_RECORD_FORK and PERF_RECORD_EXIT (as emitted by PerfDataReader.decodeBody),
extract parent and child PIDs from the event payload for FORK and the exiting
PID for EXIT, and on FORK copy the parent's PID subtree/mappings into the child
PID subtree in the state system, while on EXIT clear the exiting PID's
subtree/mappings so stale mappings are removed; keep existing MMAP/MMAP2
handling intact and use the same state-system API you already use for writing
mapping entries to perform the copy and clear operations.
Comment on lines
+79
to
+81
| if (pid < 0) { | ||
| // Skip kernel-synthesized MMAPs with pid = -1 ([kernel.kallsyms], modules, ...) | ||
| return; |
There was a problem hiding this comment.
Dropping pid == -1 removes all kernel/module mappings.
perf emits [kernel.kallsyms] and module MMAP records with pid = -1. Returning here means kernel-side callchain frames can never resolve through PerfDataMmapSymbolProvider and will fall back to raw IPs. If kernel stacks are meant to be supported, keep these mappings in a dedicated global bucket and let the symbol provider consult it when per-process lookup misses.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.java`
around lines 79 - 81, The current early return for pid < 0 drops kernel/module
mappings (pid == -1); instead, change the pid check so pid == -1 is NOT
discarded and is stored in a dedicated global bucket inside
PerfDataMmapStateProvider (create or reuse a globalMmaps/map for kernel
mappings) while preserving ignoring other negative pids if necessary, and update
the symbol lookup in PerfDataMmapSymbolProvider to consult that global bucket
when a per-process lookup misses; modify the mmap-handling branch where the pid
check occurs to branch on pid == -1 (store to global) vs pid >= 0 (store
per-process) and ensure lookup code queries globalMmaps as a fallback.
Comment on lines
+224
to
+229
| @Override | ||
| public ITmfContext seekEvent(double ratio) { | ||
| long span = fDataEnd - fDataStart; | ||
| long offset = fDataStart + (long) Math.max(0.0, Math.min(1.0, ratio)) * span; | ||
| return seekEvent(new TmfLongLocation(offset)); | ||
| } |
There was a problem hiding this comment.
Operator precedence causes incorrect ratio calculation.
Line 227 has incorrect operator precedence. The expression (long) Math.max(...) * span casts only the ratio result to long before multiplying by span. Since the ratio is between 0.0 and 1.0, casting it to long always yields 0, making offset always equal to fDataStart.
Proposed fix
`@Override`
public ITmfContext seekEvent(double ratio) {
long span = fDataEnd - fDataStart;
- long offset = fDataStart + (long) Math.max(0.0, Math.min(1.0, ratio)) * span;
+ long offset = fDataStart + (long) (Math.max(0.0, Math.min(1.0, ratio)) * span);
return seekEvent(new TmfLongLocation(offset));
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfDataTrace.java`
around lines 224 - 229, In seekEvent(double ratio) the multiplication is done
after casting the clamped ratio to long, so (long) Math.max(...) becomes 0 for
ratios <1; change the calculation in PerfDataTrace.seekEvent so you multiply the
clamped double ratio by span first and only then cast to long (e.g. compute
(long)(clampedRatio * span) or use DoubleMath) to produce the correct offset
passed to new TmfLongLocation(offset).
Comment on lines
+28
to
+29
| private static final Map<Integer, ITmfEventType> TYPES = new HashMap<>(); | ||
|
|
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
find . -name "PerfEventType.java" -type fRepository: eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator
Length of output: 250
🏁 Script executed:
git ls-files | grep -i "PerfEventType"Repository: eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator
Length of output: 248
🏁 Script executed:
fd "PerfEventType" --type fRepository: eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator
Length of output: 248
🏁 Script executed:
wc -l tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfEventType.javaRepository: eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator
Length of output: 251
🏁 Script executed:
cat -n tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfEventType.javaRepository: eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator
Length of output: 4158
Protect dynamic event-type caching from concurrent mutation.
lookup() performs an unsynchronized read-then-write on TYPES for unknown types. Since this is a public static API likely called concurrently during trace processing, use ConcurrentHashMap with computeIfAbsent to safely cache unknown event types.
Suggested fix
-import java.util.HashMap;
+import java.util.concurrent.ConcurrentHashMap;
import java.util.Map;
@@
- private static final Map<Integer, ITmfEventType> TYPES = new HashMap<>();
+ private static final Map<Integer, ITmfEventType> TYPES = new ConcurrentHashMap<>();
@@
public static ITmfEventType lookup(int type) {
- ITmfEventType t = TYPES.get(type);
- if (t == null) {
- t = new TmfEventType("PERF_RECORD_" + type, null); //$NON-NLS-1$
- TYPES.put(type, t);
- }
- return t;
+ return TYPES.computeIfAbsent(type, t -> new TmfEventType("PERF_RECORD_" + t, null)); //$NON-NLS-1$
}🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfEventType.java`
around lines 28 - 29, The static TYPES map in PerfEventType is currently a
HashMap and lookup() does an unsynchronized read-then-write, causing concurrent
mutation risk; change TYPES to a ConcurrentHashMap and update lookup() to use
TYPES.computeIfAbsent(key, k -> /* create new ITmfEventType for k */) so unknown
event types are safely cached atomically; reference the PerfEventType class, the
TYPES field, and the lookup() method when making these changes.
Port the CTF-based perf-profiling analyses to the new perf.data trace
type so the Flame Chart and Flame Graph views light up on
'perf record -g' output.
New analyses (org.eclipse.tracecompass.incubator.internal.perf.core.*):
- analysis.PerfDataCallchainAnalysisModule
Extends ProfilingCallGraphAnalysisModule and implements
ISamplingDataProvider. Reads the callchain long[] straight off
the PerfRecord attached to each PERF_RECORD_SAMPLE event,
strips the PERF_CONTEXT_* sentinels and reverses the chain
(oldest frame first), then groups samples by process/thread.
- symbol.PerfDataMmapStateProvider + PerfDataMmapAnalysisModule
Tracks PERF_RECORD_MMAP / MMAP2 events and exposes
/<pid>/<loadBase> -> filename so addresses can be resolved to
ELF libraries. Uses addr - pgoff as the load base to match the
offsets the callchain IPs will be compared against.
- symbol.PerfDataMmapSymbolProvider + factory + TmfLibrarySymbol
ISymbolProvider backed by the mmap state system and
IMappingFile; falls back to the filename when a symbol cannot
be resolved from disk.
Infrastructure:
- MANIFEST.MF: require o.e.tc.statesystem.core and
o.e.tc.analysis.profiling.core.
- plugin.xml: register the two analyses and the symbol provider
factory, scoped to PerfDataTrace.
- plugin.properties: names for the two analyses.
- PerfDataTrace: stop stringifying long[] and byte[] fields in
fieldsFromRecord so analyses can consume the primitive arrays.
Tests: - Add a second fixture traces/perf-callgraph.data captured with
'perf record -g ping 4.4.4.4 -c 3'.
- PerfDataCallchainTest.hasNonTrivialCallchains checks the data
shape on the original wget trace.
- PerfDataCallchainTest.callgraphTraceHasRealChains walks the new
fixture and verifies every SAMPLE exposes a long[] callchain
with non-sentinel IPs.
This code was made with the assistance of Claude Opus 4.7
The commit message was 100% made by claude and is much better than I can do :)
Change-Id: Ibd37279cc63ce4a0bcada7a49ec2414f4e7157d2
Signed-off-by: Matthew Khouzam <matthew.khouzam@ericsson.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What it does
Adds a new
perf.datatrace type that parses Linuxperf.datafiles directly, and builds flame chart / flame graph analyses on top of it.perf.datafile.How to test
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
perf.datatrace files as a new trace type in Eclipse Trace Compass.Tests