Perf#269
Open
MatthewKhouzam wants to merge 2 commits into
Open
Conversation
📝 WalkthroughWalkthroughAdds a perf.data trace type: a random-access PerfDataReader and models, a TmfTrace integration, callchain and mmap analyses, mmap-based symbol provider, JUnit tests, and Eclipse/Maven project metadata for core and test bundles. Changesperf.core + tests
Sequence DiagramsequenceDiagram
participant User
participant TMF as TMF/Trace
participant PerfDataTrace as PerfDataTrace
participant Reader as PerfDataReader
participant Analysis as CallchainAnalysis
participant StateSystem as MMAPStateSystem
participant SymbolProvider as PerfDataMmapSymbolProvider
User->>TMF: Open perf.data file
TMF->>PerfDataTrace: initTrace(path)
PerfDataTrace->>Reader: new PerfDataReader(file)
Reader->>Reader: Parse header, attrs, features
Note over Reader: Determine data offset/size and default attrs
User->>TMF: Iterate events
TMF->>PerfDataTrace: parseEvent(context)
PerfDataTrace->>Reader: readRecordAt(offset)
Reader->>PerfDataTrace: PerfRecord
PerfDataTrace->>TMF: Emit TmfEvent
TMF->>Analysis: Pass event
alt SAMPLE record
Analysis->>Analysis: Extract callchain, filter sentinels, build stacks
Analysis-->>TMF: Aggregated call sites / call stack data
else MMAP/MMAP2 record
Analysis->>StateSystem: Write load_base for pid
StateSystem-->>Analysis: State updated
end
User->>SymbolProvider: Resolve symbol(pid,timestamp,address)
SymbolProvider->>StateSystem: Query MMAP intervals
StateSystem-->>SymbolProvider: filename@timestamp
SymbolProvider->>SymbolProvider: Load ELF, lookup symbol
SymbolProvider-->>User: TmfLibrarySymbol / TmfResolvedSymbol
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
|
![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
+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
+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.
MatthewKhouzam
force-pushed
the
perf
branch
2 times, most recently
from
20cdd4f to
e08f321
Compare
arfio
requested changes
May 21, 2026
arfio
left a comment
arfio
left a comment
Contributor
There was a problem hiding this comment.
Thank you for adding perf files, I think many people will benefit from this addition!
You may want to add author tag to the classes you created, otherwise I put a few comments here and there, and upvoted pertinent AI comments.
arfio
requested changes
May 27, 2026
arfio
left a comment
arfio
left a comment
Contributor
There was a problem hiding this comment.
A few warnings and small things and then I will gladly approve!
| private static final String FIELD_PID = "pid"; //$NON-NLS-1$ | ||
| private static final String FIELD_TID = "tid"; //$NON-NLS-1$ | ||
|
|
||
| private final CallStackGroupDescriptor fThreadDescriptor; |
| if (reversed.length == 0) { | ||
| return null; | ||
| } | ||
| ICallStackElement element = getElement(event); |
| return null; | ||
| } | ||
|
|
||
| private static int extractInt(ITmfEvent event, String field, int defaultValue) { |
| return defaultValue; | ||
| } | ||
|
|
||
| private ICallStackElement getElement(ITmfEvent event) { |
Contributor
There was a problem hiding this comment.
NonNull ICallStackElement getElement
| } | ||
| } | ||
|
|
||
| private void handleEvent(ITmfEvent event) { |
| * (pid, time, address), it looks up the best matching mmap entry and | ||
| * resolves the address inside the backing ELF (if it can be read from disk). | ||
| */ | ||
| public class PerfDataMmapSymbolProvider implements ISymbolProvider { |
Contributor
There was a problem hiding this comment.
Please fix NonNull warnings in this class l125, l138, l95
| // Aspects | ||
| // --------------------------------------------------------------------- | ||
|
|
||
| private static final Collection<ITmfEventAspect<?>> ASPECTS = ImmutableList.of( |
Contributor
There was a problem hiding this comment.
Add NonNull here, l311, l340
| * @param sourceFile | ||
| * the backing file of this symbol | ||
| */ | ||
| public TmfLibrarySymbol(long address, String name, String sourceFile) { |
Contributor
There was a problem hiding this comment.
Add nonNull for the name argument
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
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>
There was a problem hiding this comment.
Actionable comments posted: 3
♻️ Duplicate comments (3)
tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.java (1)
91-94:⚠️ Potential issue | 🟠 Major | 🏗️ Heavy liftKeep kernel/module mappings instead of dropping
pid == -1.
perfusespid == -1for[kernel.kallsyms]and module MMAP records, so this early return guarantees kernel frames will miss symbol resolution and fall back to raw IPs. Store those mappings in a dedicated global bucket and let the symbol provider consult it after per-process lookup misses.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.java` around lines 91 - 94, The code in PerfDataMmapStateProvider currently returns early when pid < 0, which drops perf records with pid == -1 (kernel/module mappings); remove that early return and instead detect pid == -1 and store those mmap mappings in a dedicated global kernel/module bucket (e.g., a new map field on PerfDataMmapStateProvider). Continue to handle regular per-process mmap storage for pid >= 0 as before, and update the symbol lookup flow in the symbol provider to consult the new global kernel/module bucket after per-process lookup misses so kernel frames can resolve symbols.tracetypes/org.eclipse.tracecompass.incubator.perf.core/about.html (1)
19-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwitch the external links to HTTPS.
Both anchors still point to plaintext HTTP URLs. Use HTTPS for the EPL and Eclipse links in the distributed about page.
Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/about.html` at line 19, The about.html anchors use plaintext HTTP; update both anchor hrefs and visible URLs to use HTTPS (change "http://www.eclipse.org/legal/epl-2.0" to "https://www.eclipse.org/legal/epl-2.0" and any other "http://www.eclipse.org" occurrences) in the about.html file (the anchor snippet shown as available at <a href="http://www.eclipse.org/legal/epl-2.0">…</a> and the other anchors around lines 31-32) so both the href attributes and the displayed link text use secure HTTPS URLs.tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/about.html (1)
19-19:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winSwitch the external links to HTTPS.
Both anchors still point to plaintext HTTP URLs. Use HTTPS for the EPL and Eclipse links in the distributed about page.
Also applies to: 31-32
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/about.html` at line 19, Update the two anchor hrefs that use plaintext HTTP to use HTTPS: change "http://www.eclipse.org/legal/epl-2.0" to "https://www.eclipse.org/legal/epl-2.0" and likewise update the other Eclipse link(s) referred to at lines 31-32 from "http://www.eclipse.org" (or its full href) to "https://www.eclipse.org"; ensure both anchor tags in the about.html file consistently use HTTPS.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`:
- Around line 293-306: The code in PerfDataReader that parses the
perf_event_attr fixed prefix must validate that the ByteBuffer has at least the
minimum fixed-size bytes before reading fields to avoid
BufferUnderflowException; update the parsing routine (the block using
buf.position(), buf.getInt(), buf.getLong() etc. that reads type,
attrStructSize, config, samplePeriod, sampleType, readFormat, flags,
wakeup_events, bp_type, bp_addr/config1, bp_len/config2, branchSampleType) to
first compute/verify buf.remaining() >= MIN_ATTR_PREFIX_SIZE (the known
fixed-size size of the prefix) and if not throw an IOException (with a clear
message) instead of proceeding to read, then continue existing reads only after
the check passes.
- Around line 622-630: The decoder currently always uses fDefaultAttr in
decodeSample(...) and decodeTrailingSampleId(...), causing records from traces
with multiple perf_event_attr layouts to be parsed with the wrong
sample_type/sample_id_all; add and maintain a mapping from attr ID to its
PerfEventAttr (populated when parsing attr sections) and, in both decodeSample
and decodeTrailingSampleId, look up the record's attr ID and use that
PerfEventAttr instead of fDefaultAttr (fall back to fDefaultAttr only if the
lookup misses); update code paths referenced around decodeSample,
decodeTrailingSampleId and the attr-parsing logic (the attr-section handler that
fills the attr registry) to store and retrieve by attr ID so each record is
decoded with its matching PerfEventAttr.
- Around line 168-180: readRecordAt(...) currently calls readBytes(...) twice
which memory-maps the header and body per-event; change it to use a long-lived
mapped/sliced ByteBuffer or a reusable ByteBuffer/read via FileChannel.read so
you don't create short-lived mapped buffers for each record. Specifically,
replace the two readBytes(...) calls in readRecordAt (used to obtain hdr and
body using PerfConstants.PERF_EVENT_HEADER_SIZE and bodyLen) with reads from a
single pre-mapped data section ByteBuffer (or a single reusable ByteBuffer) and
use slice/position/limit to get the header and then the body while preserving
fHeader.getOrder(); apply the same change to the similar block at the other
occurrence (lines ~771-779) and keep the existing size checks (MAX_RECORD_SIZE,
dataEnd) logic intact.
---
Duplicate comments:
In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/about.html`:
- Line 19: Update the two anchor hrefs that use plaintext HTTP to use HTTPS:
change "http://www.eclipse.org/legal/epl-2.0" to
"https://www.eclipse.org/legal/epl-2.0" and likewise update the other Eclipse
link(s) referred to at lines 31-32 from "http://www.eclipse.org" (or its full
href) to "https://www.eclipse.org"; ensure both anchor tags in the about.html
file consistently use HTTPS.
In `@tracetypes/org.eclipse.tracecompass.incubator.perf.core/about.html`:
- Line 19: The about.html anchors use plaintext HTTP; update both anchor hrefs
and visible URLs to use HTTPS (change "http://www.eclipse.org/legal/epl-2.0" to
"https://www.eclipse.org/legal/epl-2.0" and any other "http://www.eclipse.org"
occurrences) in the about.html file (the anchor snippet shown as available at <a
href="http://www.eclipse.org/legal/epl-2.0">…</a> and the other anchors around
lines 31-32) so both the href attributes and the displayed link text use secure
HTTPS URLs.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapStateProvider.java`:
- Around line 91-94: The code in PerfDataMmapStateProvider currently returns
early when pid < 0, which drops perf records with pid == -1 (kernel/module
mappings); remove that early return and instead detect pid == -1 and store those
mmap mappings in a dedicated global kernel/module bucket (e.g., a new map field
on PerfDataMmapStateProvider). Continue to handle regular per-process mmap
storage for pid >= 0 as before, and update the symbol lookup flow in the symbol
provider to consult the new global kernel/module bucket after per-process lookup
misses so kernel frames can resolve symbols.
🪄 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: f7247786-8d3d-4064-8d01-3ed2ef377d6c
📒 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
✅ Files skipped from review due to trivial changes (21)
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.core.runtime.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.classpath
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.gitignore
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.classpath
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.gitignore
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.core.resources.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/plugin.properties
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.project
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.pde.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.pde.api.tools.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.jdt.ui.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.jdt.ui.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/plugin.properties
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.core.runtime.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/package-info.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.project
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/package-info.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.core.resources.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/package-info.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/package-info.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.pde.prefs
🚧 Files skipped from review as they are similar to previous changes (21)
- tracetypes/pom.xml
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/TmfLibrarySymbol.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/.settings/org.eclipse.jdt.core.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfEventAttr.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/build.properties
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/META-INF/MANIFEST.MF
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/build.properties
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapAnalysisModule.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfFileSection.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/plugin.xml
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/META-INF/MANIFEST.MF
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfEventType.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/analysis/PerfDataCallchainAnalysisModule.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfRecord.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataTraceTest.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfFileHeader.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/.settings/org.eclipse.jdt.core.prefs
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/trace/PerfDataTrace.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core.tests/src/org/eclipse/tracecompass/incubator/perf/core/tests/PerfDataCallchainTest.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/symbol/PerfDataMmapSymbolProvider.java
- tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfConstants.java
Comment on lines
+168
to
+180
| ByteBuffer hdr = readBytes(offset, PerfConstants.PERF_EVENT_HEADER_SIZE).order(fHeader.getOrder()); | ||
| int type = hdr.getInt(); | ||
| int misc = hdr.getShort() & 0xffff; | ||
| int size = hdr.getShort() & 0xffff; | ||
| if (size < PerfConstants.PERF_EVENT_HEADER_SIZE || size > MAX_RECORD_SIZE) { | ||
| throw new IOException("Invalid record size " + size + " at offset " + offset); //$NON-NLS-1$ //$NON-NLS-2$ | ||
| } | ||
| if (offset + size > dataEnd) { | ||
| // truncated record; stop here | ||
| return null; | ||
| } | ||
| int bodyLen = size - PerfConstants.PERF_EVENT_HEADER_SIZE; | ||
| ByteBuffer body = readBytes(offset + PerfConstants.PERF_EVENT_HEADER_SIZE, bodyLen); |
There was a problem hiding this comment.
Avoid memory-mapping every record read.
readRecordAt(...) maps the header and body separately for every event, so a full scan of a large trace creates a huge number of short-lived mapped buffers on the hot path. That adds avoidable VM/memory pressure and can become a real bottleneck for the flame graph use case. Prefer FileChannel.read(...) with a reusable buffer, or map the data section once and slice it.
Also applies to: 771-779
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`
around lines 168 - 180, readRecordAt(...) currently calls readBytes(...) twice
which memory-maps the header and body per-event; change it to use a long-lived
mapped/sliced ByteBuffer or a reusable ByteBuffer/read via FileChannel.read so
you don't create short-lived mapped buffers for each record. Specifically,
replace the two readBytes(...) calls in readRecordAt (used to obtain hdr and
body using PerfConstants.PERF_EVENT_HEADER_SIZE and bodyLen) with reads from a
single pre-mapped data section ByteBuffer (or a single reusable ByteBuffer) and
use slice/position/limit to get the header and then the body while preserving
fHeader.getOrder(); apply the same change to the similar block at the other
occurrence (lines ~771-779) and keep the existing size checks (MAX_RECORD_SIZE,
dataEnd) logic intact.
Comment on lines
+293
to
+306
| int startPos = buf.position(); | ||
| int type = buf.getInt(); | ||
| int attrStructSize = buf.getInt(); | ||
| long config = buf.getLong(); | ||
| long samplePeriod = buf.getLong(); | ||
| long sampleType = buf.getLong(); | ||
| long readFormat = buf.getLong(); | ||
| long flags = buf.getLong(); | ||
| buf.getInt(); // wakeup_events | ||
| buf.getInt(); // bp_type | ||
| buf.getLong(); // bp_addr / config1 | ||
| buf.getLong(); // bp_len / config2 | ||
| long branchSampleType = buf.getLong(); | ||
|
|
There was a problem hiding this comment.
Validate the fixed perf_event_attr prefix before reading it.
A truncated or corrupt attr entry will currently throw BufferUnderflowException here instead of failing as an IOException, which makes trace open fail with an unexpected runtime exception. Guard the minimum fixed-size prefix and reject short attrs explicitly before consuming fields.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`
around lines 293 - 306, The code in PerfDataReader that parses the
perf_event_attr fixed prefix must validate that the ByteBuffer has at least the
minimum fixed-size bytes before reading fields to avoid
BufferUnderflowException; update the parsing routine (the block using
buf.position(), buf.getInt(), buf.getLong() etc. that reads type,
attrStructSize, config, samplePeriod, sampleType, readFormat, flags,
wakeup_events, bp_type, bp_addr/config1, bp_len/config2, branchSampleType) to
first compute/verify buf.remaining() >= MIN_ATTR_PREFIX_SIZE (the known
fixed-size size of the prefix) and if not throw an IOException (with a clear
message) instead of proceeding to read, then continue existing reads only after
the check passes.
Comment on lines
+622
to
+630
| private long decodeSample(ByteBuffer body, Map<String, Object> fields) { | ||
| PerfEventAttr attr = fDefaultAttr; | ||
| if (attr == null) { | ||
| byte[] raw = new byte[body.remaining()]; | ||
| body.get(raw); | ||
| fields.put("raw", raw); //$NON-NLS-1$ | ||
| return 0L; | ||
| } | ||
| long sampleType = attr.getSampleType(); |
There was a problem hiding this comment.
Do not decode every record with the first attr.
Both decodeSample(...) and decodeTrailingSampleId(...) key off fDefaultAttr, so any perf.data file with multiple perf_event_attr layouts will parse later records with the wrong sample_type/sample_id_all contract. That can corrupt timestamps, CPUs, IDs, and callchains for mixed-event recordings. Keep an id→attr lookup from the attr sections and select the matching attr per record before decoding.
Also applies to: 711-717
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@tracetypes/org.eclipse.tracecompass.incubator.perf.core/src/org/eclipse/tracecompass/incubator/internal/perf/core/PerfDataReader.java`
around lines 622 - 630, The decoder currently always uses fDefaultAttr in
decodeSample(...) and decodeTrailingSampleId(...), causing records from traces
with multiple perf_event_attr layouts to be parsed with the wrong
sample_type/sample_id_all; add and maintain a mapping from attr ID to its
PerfEventAttr (populated when parsing attr sections) and, in both decodeSample
and decodeTrailingSampleId, look up the record's attr ID and use that
PerfEventAttr instead of fDefaultAttr (fall back to fDefaultAttr only if the
lookup misses); update code paths referenced around decodeSample,
decodeTrailingSampleId and the attr-parsing logic (the attr-section handler that
fills the attr registry) to store and retrieve by attr ID so each record is
decoded with its matching PerfEventAttr.
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
2 participants
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
Tests