Vm flow analysis#265
Open
beliosien wants to merge 3 commits intoeclipse-tracecompass-incubator:masterfrom
Open
Vm flow analysis#265beliosien wants to merge 3 commits intoeclipse-tracecompass-incubator:masterfrom
beliosien wants to merge 3 commits intoeclipse-tracecompass-incubator:masterfrom
Conversation
Signed-off-by: philippe <beliasossim@gmail.com>
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds a VM execution flow analysis subsystem: new analysis modules, state providers, data providers, UI views, and supporting model/types; plus numerous Eclipse project descriptors and manifest/plugin registrations. Changes
Sequence Diagram(s)sequenceDiagram
participant Trace as ITmfTrace
participant KvmModule as KvmExitAnalysisModule
participant KvmProvider as KvmExitStateProvider
participant StateSys as ITmfStateSystem
participant DataProv as KvmExitRateDataProvider
participant View as KvmExitDensityView
Trace->>KvmModule: schedule()
KvmModule->>KvmProvider: createStateProvider()
KvmProvider->>Trace: iterate events
Trace-->>KvmProvider: kvm_x86_exit / kvm_x86_entry
KvmProvider->>StateSys: modify CPU/VCPU attributes (kvm_exits, kvm_state, exit_reasons)
View->>DataProv: fetchTree()
DataProv->>StateSys: query for CPUs/* and exit_reasons/*
StateSys-->>DataProv: attribute quarks/values
DataProv->>DataProv: compute rates (delta/count ÷ deltaTime)
DataProv-->>View: TmfTreeModel / ITmfXyModel
View->>View: render chart + tree
sequenceDiagram
participant Guest as Guest Trace
participant Host as Host Trace
participant VMProv as VMNativeStateProvider
participant ProcFlow as ProcessFlowInfo
participant CallProv as VMNativeCallStackStateProvider
participant StateSys as ITmfStateSystem
Guest->>VMProv: syscall_entry (workload marker)
VMProv->>ProcFlow: addGuestEvent(evt)
Host->>VMProv: kvm_x86_exit (vcpu X)
VMProv->>ProcFlow: addVMTransition(isExit=true)
Host->>VMProv: hypervisor events
VMProv->>ProcFlow: addHypervisorEvent(hyperEvt)
Guest->>VMProv: kvm_x86_entry (vcpu X)
VMProv->>ProcFlow: addVMTransition(isExit=false)
ProcFlow->>ProcFlow: finalizeFlow() -> build ExecutionSequence(s)
CallProv->>StateSys: query flow state / build callstacks
CallProv->>StateSys: push/pop call stack entries
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Poem
🚥 Pre-merge checks | ✅ 1 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (1 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: 13
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
🟠 Major comments (11)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java-82-85 (1)
82-85:⚠️ Potential issue | 🟠 MajorRemove unnecessary and unsafe concrete cast to
TmfEventField.The
event.getContent()method returnsITmfEventField, which already provides thegetFieldValue()method. The concrete cast is unsafe and contradicts the pattern used throughout the codebase. Use the interface type directly instead.Proposed fix
-import org.eclipse.tracecompass.tmf.core.event.TmfEventField; +import org.eclipse.tracecompass.tmf.core.event.ITmfEventField; ... - TmfEventField content = (TmfEventField) event.getContent(); + ITmfEventField content = event.getContent();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java` around lines 82 - 85, Replace the unsafe concrete cast to TmfEventField by using the interface type ITmfEventField from event.getContent(); i.e., change the declaration that currently casts to TmfEventField and instead assign the result to an ITmfEventField, then call getFieldValue(Long.class, "cnt") and getFieldValue(Long.class, "vm_uid") on that interface instance so variables cnt and vmUid are obtained without the concrete cast.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java-163-181 (1)
163-181:⚠️ Potential issue | 🟠 MajorFilter native events by
processNamebefore appending them.Unlike
addGuestEvent, this path never checksprocessName, so a per-processProcessFlowInfocan absorb unrelated native events. It also skipsthreadsByTidbookkeeping, which makes native thread counts andisMultiThreaded()inaccurate.Suggested fix
public void addEvent(KernelEventInfo evt) { if (trackHypervisor) { return; // we don't want to track the hypervisor because we are on the native system } + + if (!processName.equals(evt.processName)) { + return; + } // For native systems, if we have a target thread, only track that thread if (targetThreadId != null && !targetThreadId.equals(evt.tid)) { return; } + + ThreadFlowInfo threadInfo = threadsByTid.computeIfAbsent(evt.tid, + k -> new ThreadFlowInfo(k, processName)); + threadInfo.addEvent(evt); String key = "NATIVE" + "_" + evt.timestamp + "_" + evt.cpuid + "_" + evt.tid; //$NON-NLS-1$ //$NON-NLS-2$ //$NON-NLS-3$ //$NON-NLS-4$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 163 - 181, In ProcessFlowInfo.addEvent, filter native events by the instance's processName (like addGuestEvent does) before creating/appending the FlowEvent and skip unrelated events; if the event's processName matches, also update threadsByTid (or equivalent thread-tracking map) the same way addGuestEvent does so native thread counts and isMultiThreaded() remain correct, then proceed to create the FlowEvent (FlowEventType.NATIVE), add the seenTransitions key and unifiedFlow entry as currently implemented.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeComparisonAnalysis.java-26-33 (1)
26-33:⚠️ Potential issue | 🟠 MajorAdd
canExecute()guard to match pattern of similar analyses and prevent exception path.The analysis is already constrained to
TmfExperimentvia plugin.xml registration, but lacks an explicitcanExecute()override. Similar analyses in the codebase (e.g.,DifferentialSeqCallGraphAnalysis,TraceEventCounterAnalysis) use explicit guards for this check. Add acanExecute()override to follow the established pattern and prevent late failures.Suggested fix
+ `@Override` + public boolean canExecute(ITmfTrace trace) { + return trace instanceof TmfExperiment; + } + `@Override` protected `@NonNull` ITmfStateProvider createStateProvider() { ITmfTrace trace = getTrace();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeComparisonAnalysis.java` around lines 26 - 33, Add an override of canExecute() in VMNativeComparisonAnalysis that mirrors the guard used in createStateProvider(): retrieve the trace via getTrace() and return true only if it is an instance of TmfExperiment; otherwise return false. This prevents the IllegalStateException path in createStateProvider() and matches the pattern used by DifferentialSeqCallGraphAnalysis and TraceEventCounterAnalysis; reference the VMNativeComparisonAnalysis class, the canExecute() method to add, and existing createStateProvider()/VMNativeStateProvider/TmfExperiment checks to implement the guard.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/StateProvider.java-24-25 (1)
24-25:⚠️ Potential issue | 🟠 MajorUse a VM-flow-specific provider ID.
Line 24still references...internal.overhead.core.analysis; this looks copy-pasted and can cause identifier collisions/confusion across analyses.Suggested fix
-private static final `@NonNull` String PROVIDER_ID = "org.eclipse.tracecompass.incubator.internal.overhead.core.analysis"; //$NON-NLS-1$ +private static final `@NonNull` String PROVIDER_ID = "org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.stateprovider"; //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/StateProvider.java` around lines 24 - 25, The PROVIDER_ID constant in StateProvider is using the wrong string ("...internal.overhead.core.analysis"); update the PROVIDER_ID value to a VM-flow-specific identifier (e.g., include "virtual.machine.analysis.flow" or similar) so it uniquely identifies this analysis; locate the static field PROVIDER_ID in the StateProvider class and replace the copy-pasted overhead ID with the new VM-flow-specific provider ID (leave VERSION unchanged).vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java-347-368 (1)
347-368:⚠️ Potential issue | 🟠 MajorPotential NPE when comparing
pidwithnativeStart.pid.At line 349,
pidcan benull(returned bygetIntField), but the comparisonpid == this.nativeStart.pidwould throw NPE when unboxing. The same issue exists at line 371.🐛 Proposed fix
if (source == TraceType.NATIVE_KERNEL) { if (this.nativeStart != null) { - if (pid == this.nativeStart.pid) { + if (pid != null && pid.intValue() == this.nativeStart.pid) { // ... rest of codeApply similar fix at line 371 for
vmStart.pid.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` around lines 347 - 368, The comparison against nativeStart.pid (and similarly vmStart.pid) can NPE when pid is null; update the checks in VMNativeStateProvider so they perform a null-safe comparison (e.g., check pid != null before comparing, or use Objects.equals(pid, this.nativeStart.pid)/Objects.equals(pid, this.vmStart.pid)) where the code currently does "pid == this.nativeStart.pid" and later "pid == this.vmStart.pid"; ensure both occurrences are fixed and keep the surrounding logic (flows lookup, ss.modifyAttribute, event count) unchanged.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java-88-98 (1)
88-98:⚠️ Potential issue | 🟠 MajorCaching mechanism is incomplete -
fCachedis never set.The
fCachedfield is checked at the beginning ofgetTree()but is never assigned. This means caching never works, and the tree is rebuilt on every call.🐛 Proposed fix
`@Override` protected TmfTreeModel<TmfTreeDataModel> getTree(ITmfStateSystem ss, Map<String, Object> parameters, `@Nullable` IProgressMonitor monitor) { // Return cached tree if available since CPU topology is static if (fCached != null) { TmfTreeModel<TmfTreeDataModel> cachedModel = fCached.getModel(); if (cachedModel != null) { return cachedModel; } } List<TmfTreeDataModel> nodes = new ArrayList<>(); // ... rest of the method ... TmfTreeModel<TmfTreeDataModel> treeModel = new TmfTreeModel<>(Collections.emptyList(), nodes); + fCached = new TmfModelResponse<>(treeModel, org.eclipse.tracecompass.tmf.core.response.ITmfResponse.Status.COMPLETED, ""); //$NON-NLS-1$ return treeModel; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java` around lines 88 - 98, The getTree(...) method checks fCached but never assigns it, so caching never takes effect; after you build the TmfTreeModel<TmfTreeDataModel> in getTree (the code path that creates the tree when fCached is null), wrap that model in the same cache holder type used elsewhere and assign it to fCached before returning (i.e., set fCached = new <cache-holder>(model) or use the existing cache factory used in this class), ensuring subsequent calls to getTree return fCached.getModel(); keep fCached and getTree identifiers consistent with their current declarations.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java-80-99 (1)
80-99:⚠️ Potential issue | 🟠 MajorDon't lock the whole provider to the first trace type.
environmentTypeis latched from the first event and then reused for the whole experiment. In the mixed native + VM setup from this PR, whichever trace arrives first causes the other branch to be ignored entirely. Decide per trace/event, or keep the mode state per trace instead of globally.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 80 - 99, The code latches environmentType on the first event (field environmentType) causing all subsequent traces to use that same mode and ignoring mixed native+VM experiments; change the logic in considerEvent to decide per-trace instead of using a single global field: either replace the single environmentType with a Map keyed by trace identity (e.g., trace name or trace.getUUID()) and store/lookup EnvironmentType there, or compute the environment type for each incoming event from event.getTrace().getName() on the fly, then call the existing helper methods considerVirtualizedEvent(...) or considerNativeEvent(...) accordingly (ensure you update any initialization code that set the old environmentType). Use the existing symbols considerEvent, environmentType, considerVirtualizedEvent, and considerNativeEvent to locate and implement the change.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeTimeGraphDataProvider.java-138-140 (1)
138-140:⚠️ Potential issue | 🟠 MajorFix duration calculation to account for inclusive intervals.
The duration should be
interval.getEndTime() - interval.getStartTime() + 1to match Trace Compass's inclusive interval semantics. Without the+1, single-timestamp states become zero-length and all other states lose their final timestamp. This pattern is used consistently throughout the codebase (e.g., SpanLifeDataProvider, RosMessageFlowDataProvider, and many others).🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeTimeGraphDataProvider.java` around lines 138 - 140, The TimeGraphState duration in VMNativeTimeGraphDataProvider is computed as interval.getEndTime() - interval.getStartTime(), but Trace Compass intervals are inclusive; update the TimeGraphState constructor call in VMNativeTimeGraphDataProvider where TimeGraphState is created (the block constructing ITimeGraphState timeGraphState = new TimeGraphState(...)) to use interval.getEndTime() - interval.getStartTime() + 1 so single-timestamp intervals are not zero-length and final timestamps are preserved.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeTimeGraphDataProvider.java-240-251 (1)
240-251:⚠️ Potential issue | 🟠 MajorFix
itemsvariable to extract selected items, not times.Line 243 incorrectly assigns
itemsfromextractTimeRequested(fetchParameters), causing the filter to be built from time values instead of selected items. This results in invalid queries that fall back to scanning the entire trace.Replace with
DataProviderParameterUtils.extractSelectedItems(fetchParameters), or use theFetchParametersUtils.createSelectionTimeQuery(fetchParameters)helper method as done in similar data providers throughout the codebase.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeTimeGraphDataProvider.java` around lines 240 - 251, In createSelectionTimeQuery (VMNativeTimeGraphDataProvider) the local variable items is incorrectly populated via DataProviderParameterUtils.extractTimeRequested; replace that call with DataProviderParameterUtils.extractSelectedItems(fetchParameters) so the SelectionTimeQueryFilter is built from selected items rather than time values, or alternately replace the entire method body with a call to FetchParametersUtils.createSelectionTimeQuery(fetchParameters) to reuse the shared helper; ensure SelectionTimeQueryFilter is still constructed with startTime, endTime and the correct items list.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java-99-102 (1)
99-102:⚠️ Potential issue | 🟠 MajorAllocate stable series IDs per CPU to prevent selection loss during trace analysis.
The tree is rebuilt every
fetchTree()call while the state system is still running (status RUNNING), allocating new IDs viaTRACE_IDS.getAndIncrement()and overwritingfCpuIdToSeriesId. When a user selects a series from an interim tree, the nextfetchTree()call assigns different IDs to the same CPUs, sofetchXY()no longer finds matches for the selected series IDs.Use deterministic IDs per CPU (e.g., derived from CPU number) instead of an incrementing counter, or cache the running tree response to preserve ID mappings across interim calls.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java` around lines 99 - 102, The current fetchTree method rebuilds the tree on each call (even while the state system is RUNNING) and uses TRACE_IDS.getAndIncrement() to assign series IDs, which updates fCpuIdToSeriesId and breaks selection continuity; change the logic in fetchTree (and the related fCpuIdToSeriesId mapping) to assign deterministic, stable IDs per CPU (for example derive IDs from the CPU number) or return a cached TmfModelResponse stored in fCached while the state system is RUNNING so IDs do not change between interim calls; update any consumers like fetchXY that match series IDs to rely on the stable per-CPU IDs or the cached response instead of the incrementing TRACE_IDS.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphDataProvider.java-171-185 (1)
171-185:⚠️ Potential issue | 🟠 MajorMirror the
fetchTree()status logic infetchRowModel()to avoid premature completion reporting.
fetchRowModel()skipswaitForInitialization()andss.waitUntilBuilt()checks, always returningStatus.COMPLETEDimmediately. This causes the UI to stop polling before the state system finishes building, potentially rendering incomplete data as final. ThefetchTree()method (lines 143–163) properly handles this by callingwaitForInitialization(), checkingss.waitUntilBuilt(0), and returningStatus.RUNNINGuntil the analysis is complete. Apply the same pattern tofetchRowModel(): add the initialization wait, check if the state system is fully built, and returnStatus.RUNNINGuntil it is.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphDataProvider.java` around lines 171 - 185, fetchRowModel currently returns Status.COMPLETED without waiting for module/state system readiness; mirror fetchTree's logic by calling waitForInitialization() at the start of fetchRowModel, then after obtaining ITmfStateSystem ss call ss.waitUntilBuilt(0) and if it returns false return a TmfModelResponse with Status.RUNNING and CommonStatusMessage.RUNNING (same as fetchTree), otherwise proceed to call getDefaultRowModels and return COMPLETED; ensure you keep the same exception handling (IndexOutOfBoundsException | TimeRangeException | StateSystemDisposedException) and null checks as before so behavior matches fetchTree.
🟡 Minor comments (7)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java-31-31 (1)
31-31:⚠️ Potential issue | 🟡 MinorFix mismatched state provider ID.
The ID
"org.eclipse.tracecompass.incubator.internal.overhead.core.analysis"appears to be copy-pasted from another module. It should reflect this KVM exit analysis module.🐛 Proposed fix
- private static final String ID = "org.eclipse.tracecompass.incubator.internal.overhead.core.analysis"; //$NON-NLS-1$ + private static final String ID = "org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.kvmexit"; //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java` at line 31, The state provider ID constant is incorrect for this module: update the private static final String ID in KvmExitStateProvider to a module-correct value that reflects the KVM exit analysis (replace "org.eclipse.tracecompass.incubator.internal.overhead.core.analysis" with an ID matching this KVM exit analysis module, e.g., one containing "virtual.machine.analysis.core" or "kvm.exit.analysis"); ensure the ID string is unique and consistent with other providers in this package and leave the variable name ID and its visibility unchanged.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeComparisonView.java-69-76 (1)
69-76:⚠️ Potential issue | 🟡 MinorAdd bounds validation for state table index.
getStateTableIndexreturnstimeEvent.getValue()directly without validating that it's within the valid range (0-3). If the data provider emits an unexpected value, this could cause anArrayIndexOutOfBoundsExceptionin the rendering code.🛡️ Proposed fix
`@Override` public int getStateTableIndex(ITimeEvent event) { if (event instanceof TimeEvent) { TimeEvent timeEvent = (TimeEvent) event; - return timeEvent.getValue(); + int value = timeEvent.getValue(); + // Validate index is within state table bounds + if (value >= 0 && value < 4) { + return value; + } } return TRANSPARENT; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeComparisonView.java` around lines 69 - 76, getStateTableIndex in VMNativeComparisonView returns TimeEvent.getValue() unchecked which can be out of range and lead to ArrayIndexOutOfBoundsException during rendering; update getStateTableIndex(ITimeEvent event) to validate the extracted int (from TimeEvent.getValue()) is within the valid state table index range (0–3) before returning it, and return TRANSPARENT (or a safe default) if the value is outside that range or if event is not a TimeEvent; reference the getStateTableIndex method and TRANSPARENT constant when making the change.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java-185-191 (1)
185-191:⚠️ Potential issue | 🟡 MinorMissing exception handling for
Integer.parseInt.Line 189 calls
Integer.parseInt(words[1])without a try-catch, which will throwNumberFormatExceptionif the exit reason code is malformed.🛡️ Proposed fix
String[] words = value.split("="); //$NON-NLS-1$ if (words.length == 0 || words.length > 2) { return null; } - int code = Integer.parseInt(words[1]); - return ExitReasonMap.getExitReasonName(code); + try { + int code = Integer.parseInt(words[1]); + return ExitReasonMap.getExitReasonName(code); + } catch (NumberFormatException e) { + return "UNKNOWN_EXIT_REASON"; //$NON-NLS-1$ + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` around lines 185 - 191, The code in VMNativeStateProvider that parses the exit reason code uses Integer.parseInt(words[1]) without handling malformed input; wrap the parse in a try-catch for NumberFormatException (or validate with a numeric check) inside the method that contains this snippet, handle the error by returning null (or a sensible default) and optionally log the malformed value, and ensure you still guard access to words[1] (words.length == 2) before parsing so ExitReasonMap.getExitReasonName(code) is only called with a valid int.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeFlameGraphPresentationProvider.java-78-100 (1)
78-100:⚠️ Potential issue | 🟡 MinorHandle potential null or immutable map from super.
super.getEventHoverToolTipInfo(event)may returnnullor an immutable map. Directly callingput()on the result could throw aNullPointerExceptionorUnsupportedOperationException.🛡️ Proposed fix
`@Override` public Map<String, String> getEventHoverToolTipInfo(`@Nullable` ITimeEvent event) { - Map<String, String> tooltipInfo = super.getEventHoverToolTipInfo(event); + Map<String, String> superInfo = super.getEventHoverToolTipInfo(event); + Map<String, String> tooltipInfo = superInfo != null ? new java.util.LinkedHashMap<>(superInfo) : new java.util.LinkedHashMap<>(); if (event instanceof NamedTimeEvent namedEvent) {🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeFlameGraphPresentationProvider.java` around lines 78 - 100, The call in VMNativeFlameGraphPresentationProvider.getEventHoverToolTipInfo currently assumes super.getEventHoverToolTipInfo(event) returns a non-null, mutable Map; change it to safely handle null/immutable returns by replacing the direct use with a mutable copy — e.g., create a new HashMap initialized from the result (or an empty map if null) and then use that mutable map for put() operations; ensure the final returned map is the new mutable map so put() cannot throw NullPointerException or UnsupportedOperationException.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml-66-73 (1)
66-73:⚠️ Potential issue | 🟡 MinorMissing
automatic="false"attribute for consistency.The
VMNativeComparisonAnalysismodule is missing theautomatic="false"attribute that other modules in this file have. This could cause the analysis to auto-run on matching experiments, which may be undesired.🔧 Proposed fix
<module analysis_module="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.VMNativeComparisonAnalysis" + automatic="false" id="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.vmcomparison" name="VM vs Native Comparison"> <tracetype + applies="true" class="org.eclipse.tracecompass.tmf.core.trace.experiment.TmfExperiment"> </tracetype> </module>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml` around lines 66 - 73, The VMNativeComparisonAnalysis module declaration (analysis_module="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.VMNativeComparisonAnalysis", id="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.vmcomparison") is missing the automatic="false" attribute; update that <module> element to include automatic="false" so it matches the other modules and does not auto-run on matching experiments.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml-74-82 (1)
74-82:⚠️ Potential issue | 🟡 MinorMissing
automatic="false"attribute for consistency.The
VMNativeCallStackAnalysismodule is also missing theautomatic="false"attribute.🔧 Proposed fix
<module analysis_module="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.VMNativeCallStackAnalysis" applies_experiment="true" + automatic="false" id="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.vm.native.callstack" name="VM Native CallStack"> <tracetype + applies="true" class="org.eclipse.tracecompass.analysis.os.linux.core.trace.IKernelTrace"> </tracetype> </module>🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml` around lines 74 - 82, The module element for VMNativeCallStackAnalysis (analysis_module="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.VMNativeCallStackAnalysis", id="org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.vm.native.callstack") is missing the automatic="false" attribute; update the <module> declaration for this analysis to include automatic="false" so it matches the other modules' consistency and behaviour.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java-146-148 (1)
146-148:⚠️ Potential issue | 🟡 MinorReplace
System.err.printlnwith proper logging.Eclipse plugins should use the platform logging infrastructure (
Activator.getInstance().getLog().log(...)) rather thanSystem.err. GenericExceptioncatch also masks specific error types.♻️ Proposed fix
- } catch (Exception a) { - System.err.println("Error processing event: " + a.getMessage()); //$NON-NLS-1$ + } catch (RuntimeException e) { + // Log or handle specific exceptions appropriately + org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.Activator.getInstance().logError("Error processing event", e); //$NON-NLS-1$ }Apply similar changes to lines 311-313 and 417-419.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` around lines 146 - 148, In VMNativeStateProvider replace the System.err.println calls in the catch blocks (the catches currently written as "catch (Exception a)" around the three locations) with platform logging via Activator.getInstance().getLog().log(...) using an appropriate Status (IStatus.ERROR) and include the caught Throwable so the stacktrace is recorded; also stop catching raw Exception—narrow the catch to the specific exception types thrown by the surrounding code or, if multiple/unknown, catch Throwable as a last resort but still log it via Activator. Locate the three catch blocks in class VMNativeStateProvider (the ones that reference "Exception a" and System.err.println) and update them to use the Activator logger and more specific exception types.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b973a3ab-1d8f-44f1-8bc1-a21df5a43b18
📒 Files selected for processing (52)
analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.javaanalyses/org.eclipse.tracecompass.incubator.overhead.core.tests/.projectanalyses/org.eclipse.tracecompass.incubator.overhead.core/.projectanalyses/org.eclipse.tracecompass.incubator.overhead.ui.swtbot.tests/.projectanalyses/org.eclipse.tracecompass.incubator.overhead.ui/.projectanalyses/org.eclipse.tracecompass.incubator.overhead/.projectcallstack/org.eclipse.tracecompass.incubator.analysis.core.tests/.projectcallstack/org.eclipse.tracecompass.incubator.analysis.core/.projectcallstack/org.eclipse.tracecompass.incubator.analysis.core/.settings/org.eclipse.m2e.core.prefscallstack/org.eclipse.tracecompass.incubator.callstack.core.tests/.projectcallstack/org.eclipse.tracecompass.incubator.callstack.core/.projectcallstack/org.eclipse.tracecompass.incubator.callstack.ui.swtbot.tests/.projectcallstack/org.eclipse.tracecompass.incubator.callstack.ui/.projectdoc/org.eclipse.tracecompass.incubator.callstack.doc.user/.projectvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/META-INF/MANIFEST.MFvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/StateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/StateSystemAnalysisModule.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeComparisonAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeDataProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeTimeGraphDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/package-info.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/META-INF/MANIFEST.MFvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/TimeGraphDataProviderView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeComparisonView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeFlameGraphPresentationProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/package-info.java
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
...virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphProviderFactory.java
Outdated
Show resolved
Hide resolved
...tual/machine/analysis/core/flow/analysis/core/data/provider/VMNativeDataProviderFactory.java
Outdated
Show resolved
Hide resolved
...al/machine/analysis/core/flow/analysis/core/data/provider/VMNativeTimeGraphDataProvider.java
Outdated
Show resolved
Hide resolved
...ompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.java
Outdated
Show resolved
Hide resolved
...ncubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java
Show resolved
Hide resolved
...tor/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java
Show resolved
Hide resolved
...tor/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Outdated
Show resolved
Hide resolved
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xml
Outdated
Show resolved
Hide resolved
arfio
requested changes
Apr 1, 2026
Contributor
arfio
left a comment
arfio
left a comment
There was a problem hiding this comment.
Thank you for the additional VM analyses!
I did a quick pass. Once the small issues are fixed, you can take a look at the coderabbit comments. I upvoted the ones I agree with, the others, you can look into it and answer them. I will review them on my next pass.
analyses/org.eclipse.tracecompass.incubator.overhead.core.tests/.project
Show resolved
Hide resolved
callstack/org.eclipse.tracecompass.incubator.analysis.core/.settings/org.eclipse.m2e.core.prefs
Outdated
Show resolved
Hide resolved
...acecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.java
Outdated
Show resolved
Hide resolved
...virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java
Show resolved
Hide resolved
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
...pse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java
Show resolved
Hide resolved
...pse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java
Outdated
Show resolved
Hide resolved
...cecompass/incubator/virtual/machine/analysis/ui/flow/analysis/TimeGraphDataProviderView.java
Outdated
Show resolved
Hide resolved
...tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/VMNativeComparisonView.java
Outdated
Show resolved
Hide resolved
...ubator/virtual/machine/analysis/ui/flow/analysis/VMNativeFlameGraphPresentationProvider.java
Outdated
Show resolved
Hide resolved
Author
|
All review comments addressed, ready for another pass. |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java (1)
268-271:⚠️ Potential issue | 🔴 CriticalFlush the current sequence before starting the next
VM_ENTRY.Each new
VM_ENTRYreplacescurrentSequencewithout storing the previous one, so after the loop only the last guest/host interval survives. This issue was flagged in a previous review and remains unfixed.🐛 Proposed fix
case VM_ENTRY: + if (currentSequence != null) { + sequences.add(currentSequence); + } currentSequence = new ExecutionSequence(); currentSequence.setVmEntry(flowEvent); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 268 - 271, Before replacing currentSequence in the VM_ENTRY case, persist the existing sequence: if currentSequence is non-null and contains any recorded guest/host interval data, add it to the collection that stores completed sequences (e.g., call the method or list that collects ExecutionSequence instances) before creating a new ExecutionSequence and calling currentSequence.setVmEntry(flowEvent); ensure you do not add empty/uninitialized sequences.
🧹 Nitpick comments (15)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.java (2)
30-41: Tighten encapsulation for thread state.Line 30, Line 35, and Line 41 keep core state package-visible, and Line 63 mutates a package-visible list. This makes it easy for sibling classes to bypass invariants. Prefer
privatefields plus controlled accessors (e.g., unmodifiable view for events).Proposed refactor
import java.util.ArrayList; +import java.util.Collections; import java.util.List; +import java.util.Objects; @@ - final int tid; + private final int tid; @@ - final String processName; + private final String processName; @@ - final List<KernelEventInfo> events = new ArrayList<>(); + private final List<KernelEventInfo> events = new ArrayList<>(); @@ void addEvent(KernelEventInfo evt) { - events.add(evt); + events.add(Objects.requireNonNull(evt, "evt")); //$NON-NLS-1$ } + + int getTid() { + return tid; + } + + String getProcessName() { + return processName; + } + + List<KernelEventInfo> getEvents() { + return Collections.unmodifiableList(events); + }Also applies to: 62-64
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.java` around lines 30 - 41, ThreadFlowInfo currently exposes core state via package-visible fields tid, processName, and events and allows external mutation of events; make tid, processName, and events private, add public getters getTid() and getProcessName(), replace direct external mutation with a controlled mutator (e.g., addEvent(KernelEventInfo) or addEvents(Collection<KernelEventInfo>) implemented on ThreadFlowInfo), and change the events accessor to return an unmodifiable view (Collections.unmodifiableList(events)); update any callers that currently read or write the package-visible fields to use the new getters/mutators.
72-77: Avoid directSystem.outprinting in analysis code paths.Line 73 and Line 76 print potentially unbounded event data directly to stdout. This can flood console output on large traces and bypasses normal logging/UI channels.
Proposed refactor
- void printFlow() { - System.out.printf(" Thread %d (%d events):\n", tid, events.size()); //$NON-NLS-1$ - - for (KernelEventInfo evt : events) { - System.out.printf(" [%d] %s\n", evt.timestamp, evt.name); //$NON-NLS-1$ - } - } + String formatFlow() { + StringBuilder builder = new StringBuilder(); + builder.append(" Thread ").append(tid).append(" (").append(events.size()).append(" events):\n"); //$NON-NLS-1$ //$NON-NLS-2$ + for (KernelEventInfo evt : events) { + builder.append(" [").append(evt.timestamp).append("] ").append(evt.name).append('\n'); //$NON-NLS-1$ + } + return builder.toString(); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.java` around lines 72 - 77, The ThreadFlowInfo.printFlow method currently uses System.out.printf to emit unbounded event data; replace these direct stdout prints with the project logging mechanism (e.g., use the plugin logger/Trace Compass logging utilities) or accumulate output into a bounded String and log at a debug/trace level to avoid flooding the console. Specifically, update printFlow (class ThreadFlowInfo) to stop calling System.out.printf for "Thread %d (%d events)" and the per-event line that prints evt.timestamp and evt.name; instead format the message into a single buffer (or use the platform logger) and emit via the project's logger at an appropriate level so output can be routed/controlled by the UI/log settings. Ensure you reference fields tid and events and KernelEventInfo evt when building the message and do not retain any System.out usage.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java (3)
44-55: Remove unusedTRACE_LAYOUTScache and write-only layout capture.Line 45 is currently global mutable state with no read path in this class, so it adds complexity without effect.
Simplify `canMatchTrace` by removing unused cache
import java.util.HashSet; -import java.util.Map; import java.util.Set; -import java.util.WeakHashMap; @@ - /** Use a weak hash map so that traces can be garbage collected */ - private static final Map<ITmfTrace, IKernelAnalysisEventLayout> TRACE_LAYOUTS = new WeakHashMap<>(); - `@Override` public boolean canMatchTrace(ITmfTrace trace) { // Get the events that this trace needs to have if (!(trace instanceof IKernelTrace)) { return false; } - IKernelAnalysisEventLayout layout = ((IKernelTrace) trace).getKernelEventLayout(); - TRACE_LAYOUTS.put(trace, layout); + ((IKernelTrace) trace).getKernelEventLayout();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java` around lines 44 - 55, TRACE_LAYOUTS is an unused write-only global cache: remove the TRACE_LAYOUTS field and the TRACE_LAYOUTS.put(trace, layout) call from canMatchTrace, and simplify canMatchTrace to just check the trace type and obtain the layout if needed locally (or omit obtaining the layout entirely) so there is no global mutable state; update references to IKernelAnalysisEventLayout, TRACE_LAYOUTS.put, and canMatchTrace accordingly.
36-42: UseQemuKvmStringsconstants instead of duplicated literals.Line 38–41 and Line 72–81 duplicate canonical event IDs, which can drift from the source of truth (
QemuKvmStrings) over time.Refactor to canonical constants
import java.util.HashSet; import java.util.Map; import java.util.Set; import java.util.WeakHashMap; @@ import org.eclipse.tracecompass.tmf.core.trace.ITmfTraceWithPreDefinedEvents; import org.eclipse.tracecompass.tmf.core.trace.TmfEventTypeCollectionHelper; +import org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.model.qemukvm.QemuKvmStrings; @@ - private static final Set<String> REQUIRED_EVENTS = new HashSet<>(); - static { - REQUIRED_EVENTS.add("vmsync_hg_host"); //$NON-NLS-1$ - REQUIRED_EVENTS.add("vmsync_hg_guest"); //$NON-NLS-1$ - REQUIRED_EVENTS.add("vmsync_gh_host"); //$NON-NLS-1$ - REQUIRED_EVENTS.add("vmsync_gh_guest"); //$NON-NLS-1$ - } + private static final Set<String> REQUIRED_EVENTS = Set.of( + QemuKvmStrings.VMSYNC_HG_HOST, + QemuKvmStrings.VMSYNC_HG_GUEST, + QemuKvmStrings.VMSYNC_GH_HOST, + QemuKvmStrings.VMSYNC_GH_GUEST); @@ - if ("vmsync_hg_host".equals(evname)) { //$NON-NLS-1$ + if (QemuKvmStrings.VMSYNC_HG_HOST.equals(evname)) { return Direction.CAUSE; // Host sends - } else if ("vmsync_hg_guest".equals(evname)) { //$NON-NLS-1$ + } else if (QemuKvmStrings.VMSYNC_HG_GUEST.equals(evname)) { return Direction.EFFECT; // Guest receives } @@ - else if ("vmsync_gh_guest".equals(evname)) { //$NON-NLS-1$ + else if (QemuKvmStrings.VMSYNC_GH_GUEST.equals(evname)) { return Direction.CAUSE; // Guest sends - } else if ("vmsync_gh_host".equals(evname)) { //$NON-NLS-1$ + } else if (QemuKvmStrings.VMSYNC_GH_HOST.equals(evname)) { return Direction.EFFECT; // Host receives }Also applies to: 72-81
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java` around lines 36 - 42, The set INITIALIZATION in VmSyncLttngEventMatching uses hard-coded event ID literals (e.g., "vmsync_hg_host", "vmsync_hg_guest", "vmsync_gh_host", "vmsync_gh_guest"); replace these duplicated string literals with the canonical constants from QemuKvmStrings (use the corresponding QemuKvmStrings.VMSYNC_* constants) wherever they appear (both in the static REQUIRED_EVENTS initialization and the other duplicated block around lines 72–81) so the file references the single source of truth for those event IDs.
62-64: Prefer a non-mutating event-presence check.Line 63 mutates
traceEventsviaretainAll. A read-only check is clearer and avoids side effects if the helper’s set is reused internally.Non-mutating alternative
- Set<String> traceEvents = TmfEventTypeCollectionHelper.getEventNames(ktrace.getContainedEventTypes()); - traceEvents.retainAll(REQUIRED_EVENTS); - return !traceEvents.isEmpty(); + Set<String> traceEvents = TmfEventTypeCollectionHelper.getEventNames(ktrace.getContainedEventTypes()); + return traceEvents.stream().anyMatch(REQUIRED_EVENTS::contains);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java` around lines 62 - 64, The code currently mutates the set returned by TmfEventTypeCollectionHelper.getEventNames by calling traceEvents.retainAll(REQUIRED_EVENTS); instead of mutating, perform a read-only presence check (e.g., use Collections.disjoint(traceEvents, REQUIRED_EVENTS) or traceEvents.stream().anyMatch(REQUIRED_EVENTS::contains)) and return the boolean result; update the logic around the traceEvents variable in VmSyncLttngEventMatching to use that non-mutating check so the helper's returned set is not modified.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java (1)
35-37: Remove no-opsetTraceoverride to reduce maintenance noise.Line 35–Line 37 only delegate to
super.setTrace(trace)and don’t add behavior. Dropping this override simplifies the class and lets inherited behavior evolve without local churn.Proposed cleanup
-import org.eclipse.tracecompass.tmf.core.exceptions.TmfAnalysisException; import org.eclipse.tracecompass.tmf.core.statesystem.ITmfStateProvider; import org.eclipse.tracecompass.tmf.core.trace.ITmfTrace; @@ - `@Override` - public boolean setTrace(ITmfTrace trace) throws TmfAnalysisException { - return super.setTrace(trace); - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java` around lines 35 - 37, Remove the no-op override of setTrace(ITmfTrace) in class VMNativeCallStackAnalysis: delete the three-line method that simply returns super.setTrace(trace) so the class inherits the superclass implementation directly, leaving no behavioral change but reducing maintenance noise; ensure there are no local usages that relied on this override before committing.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java (1)
139-143: Collapse redundant parent/child branch in thread naming.Line 139–Line 143 return the exact same expression in both branches, so the conditional can be removed for clarity.
Proposed simplification
- // Label parent vs child - if (pid == nativeStart.pid) { - return getProcessName(event) + "-" + Long.toString(tid); //$NON-NLS-1$ - } return getProcessName(event) + "-" + Long.toString(tid); //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 139 - 143, The conditional branch in VMNativeCallStackStateProvider that checks "if (pid == nativeStart.pid)" is redundant because both branches return the same value; remove the if/else and replace with a single return combining getProcessName(event) and Long.toString(tid) (the expression currently returned in both branches), leaving pid, nativeStart.pid, getProcessName(event) and tid references intact.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java (3)
511-513: Unused methodisInGuest().The method
VMExecutionState.isInGuest()is defined but never called. Remove it or add a suppression comment if it's intended for future use.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` around lines 511 - 513, The VMExecutionState.isInGuest() method is unused; either remove the method from the VMExecutionState class or, if it is intentionally kept for future use, annotate it with an appropriate suppression and comment (e.g., add `@SuppressWarnings`("unused") and a brief rationale) so the unused-method warning is explicit; update VMNativeStateProvider/VMExecutionState accordingly to avoid leaving dead code.
474-477:TraceContextclass stores no state—constructor parameters are unused.The constructor accepts
traceandtypebut doesn't store them. Either store these fields or remove the class if it's not needed.♻️ Proposed fix
private static class TraceContext { + final ITmfTrace trace; + final TraceType type; + TraceContext(ITmfTrace trace, TraceType type) { + this.trace = trace; + this.type = type; } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` around lines 474 - 477, The TraceContext inner class currently ignores its constructor parameters (ITmfTrace trace, TraceType type); either persist them as fields or eliminate the class if unused—add private final fields (e.g., ITmfTrace trace; TraceType type) to VMNativeStateProvider.TraceContext and assign them in the TraceContext(ITmfTrace trace, TraceType type) constructor (and add getters if other code needs access), or remove the TraceContext type and update callers to use the trace and type directly.
13-13: Avoid wildcard imports.Line 13 uses
import java.util.*;which is discouraged. Use explicit imports for better clarity and to avoid potential naming conflicts.♻️ Proposed fix
-import java.util.*; +import java.util.HashMap; +import java.util.HashSet; +import java.util.Map; +import java.util.Set; +import java.util.regex.Matcher; +import java.util.regex.Pattern;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` at line 13, Replace the wildcard import in VMNativeStateProvider by listing only the java.util types actually referenced in that class; remove "import java.util.*;" and add explicit imports such as java.util.List, java.util.Map, java.util.Set, java.util.HashMap, java.util.ArrayList, java.util.Collections, java.util.EnumSet, java.util.Optional, java.util.Iterator, java.util.Objects (only include the exact types used by VMNativeStateProvider) to avoid wildcard imports and potential name conflicts.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java (1)
176-178: Potential integer overflow when converting Long to int.
((Long) obj).intValue()will silently truncate values exceedingInteger.MAX_VALUE. While CPU/VCPU IDs are unlikely to be that large, consider adding a bounds check or usingMath.toIntExact()to fail fast on overflow.🛡️ Proposed defensive fix
} else if (obj instanceof Long) { - return ((Long) obj).intValue(); + long val = (Long) obj; + if (val < Integer.MIN_VALUE || val > Integer.MAX_VALUE) { + return null; + } + return (int) val;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java` around lines 176 - 178, In KvmExitStateProvider change the unsafe downcast of Long to int: currently ((Long) obj).intValue() can silently truncate large values; replace it with a safe conversion (e.g. Math.toIntExact((Long) obj) or explicit bounds checks against Integer.MIN_VALUE/Integer.MAX_VALUE) so the code throws on overflow instead of producing incorrect IDs; update the branch handling in the method where this cast occurs to use the chosen safe conversion and handle the resulting ArithmeticException or out-of-range condition appropriately.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.java (2)
18-18: Using internal packageMessagesis fragile.Importing from
org.eclipse.tracecompass.internal.tmf.core.histogram.Messagescouples this code to Trace Compass internals that may change without notice. Consider defining local message constants or using a public API.♻️ Proposed fix
Define local constants:
+ private static final String COLUMN_NAME = "Name"; //$NON-NLS-1$ + private static final String COLUMN_LEGEND = "Legend"; //$NON-NLS-1$ + `@Override` protected ITmfTreeColumnDataProvider getColumnDataProvider() { return () -> { - return ImmutableList.of(createColumn(Messages.NewHistogramTree_ColumnName, Comparator.comparing(TmfGenericTreeEntry::getName)), - new TmfTreeColumnData(Messages.NewHistogramTree_Legend)); + return ImmutableList.of(createColumn(COLUMN_NAME, Comparator.comparing(TmfGenericTreeEntry::getName)), + new TmfTreeColumnData(COLUMN_LEGEND)); }; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.java` at line 18, EventDensityTreeViewer imports org.eclipse.tracecompass.internal.tmf.core.histogram.Messages which ties the class to internal APIs; replace that dependency by adding local message constants or switching to the public API equivalents and update all usages in EventDensityTreeViewer to reference the new local constants (or the public API class) instead of Messages so the code no longer imports the internal org.eclipse.tracecompass.internal package.
36-38: Unchecked cast warning should be suppressed or documented.The cast to
TmfGenericTreeEntry<TmfTreeDataModel>is unchecked. Add@SuppressWarnings("unchecked")with a comment explaining safety, or use a more defensive approach.♻️ Proposed fix
`@Override` + `@SuppressWarnings`("unchecked") public String getColumnText(Object element, int columnIndex) { if (columnIndex == 0 && element instanceof TmfGenericTreeEntry) { + // Safe cast: AbstractSelectTreeViewer2 only uses TmfGenericTreeEntry<TmfTreeDataModel> TmfGenericTreeEntry<TmfTreeDataModel> genericEntry = (TmfGenericTreeEntry<TmfTreeDataModel>) element;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.java` around lines 36 - 38, The unchecked cast warning occurs when casting element to TmfGenericTreeEntry<TmfTreeDataModel> in EventDensityTreeViewer; suppress it by adding `@SuppressWarnings`("unchecked") on the smallest scope (the method or the local variable) and add a brief comment that the suppression is safe because you first check element instanceof TmfGenericTreeEntry and the tree entry is known to contain TmfTreeDataModel instances, or alternatively replace the cast with a defensive helper that verifies the generic payload type before casting.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java (1)
291-296: NATIVE events added tohypervisorEventslist is semantically misleading.Adding
NATIVEtype events viaaddHypervisorEvent()conflates native execution with hypervisor overhead. Consider adding a dedicatedaddNativeEvent()method or a separate list for native events inExecutionSequence.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 291 - 296, The code currently treats NATIVE events the same as hypervisor events by calling ExecutionSequence.addHypervisorEvent(flowEvent) inside the NATIVE case, which is misleading; change ExecutionSequence to hold native events separately (e.g., add a new List nativeEvents and a method addNativeEvent(ITmfEvent or FlowEventType)) and update the NATIVE branch in ProcessFlowInfo to call currentSequence.addNativeEvent(flowEvent) instead of addHypervisorEvent; also update any consumers of ExecutionSequence to handle the new nativeEvents collection where appropriate and add unit/tests for the new method to preserve behavior separation.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java (1)
191-193: Silent exception swallowing hides quark lookup failures.Catching
IndexOutOfBoundsExceptionwithout logging makes debugging difficult. Consider logging at debug level or usingoptQuarkRelativewhich returnsINVALID_ATTRIBUTEinstead of throwing.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java` around lines 191 - 193, The try/catch in KvmExitRateDataProvider that swallows IndexOutOfBoundsException should be changed to use the safe lookup API and/or log failures: replace the throwing quark lookup with optQuarkRelative(...) and check for the INVALID_ATTRIBUTE sentinel (or equivalent constant) instead of catching IndexOutOfBoundsException; if you must keep the catch, log the failure at debug level including the attribute name/quark and context. Locate the quark lookup call wrapped by the catch (the IndexOutOfBoundsException handler) and update it to use optQuarkRelative and a conditional skip when it returns INVALID_ATTRIBUTE, or add a debug processLogger.debug(...) inside the catch to record the failing lookup.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java`:
- Around line 66-68: fetchXY() currently depends on the shared mutable field
fCpuIdToSeriesId which is populated as a side-effect by fetchTree(), causing
coupling and race conditions; change fetchXY() in KvmExitDataProvider to
rebuild/compute its own local Map<Integer,Long> (using the same trace/CPU
decoding logic that fetchTree() uses) and use that local map for series id
lookups so each XY request is self-contained and thread-safe, removing the
reliance on the shared fCpuIdToSeriesId (or at least avoid reading it without
synchronization).
- Around line 248-259: The current queryCumulativeCount method swallows
StateSystemDisposedException and returns 0, which can hide disposed-state
failures; change the implementation in queryCumulativeCount(ITmfStateSystem ss,
int quark, long timestamp) to not catch-and-ignore StateSystemDisposedException
(either remove the catch block or rethrow the exception) so the exception
propagates to callers (who will map it to STATE_SYSTEM_FAILED) instead of
fabricating a zero count; keep the existing logic that handles Number values and
only allow non-recoverable state system disposal to bubble up.
- Around line 103-126: The root node currently uses id fTraceId while the
aggregate "All CPUs" uses allCpusId = fTraceId * 1000 so when fTraceId == 0 both
are 0; make their IDs distinct by giving the root a scaled id (or the aggregate
an offset) and update fCpuIdToSeriesId accordingly: e.g., set the root node id
to fTraceId * 1000 + 0 (used in the TmfTreeDataModel creation) and keep
allCpusId as fTraceId * 1000 + 1 (or vice versa), then replace uses of fTraceId
for the root and set fCpuIdToSeriesId.put(-1, allCpusId) to the new unique
aggregate id so TmfTreeDataModel and TmfXyTreeDataModel ids (and the
fCpuIdToSeriesId map) no longer collide.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java`:
- Around line 273-313: updateValue can run before setPrevObservation so
fPrevCpuCounts may be empty and all rates get skipped; also the catch of generic
Exception hides specific state-system errors. Ensure updateValue initializes
fPrevCpuCounts (populate each cpuId with currentCpuCount) and sets prev time on
the first invocation instead of skipping rates so subsequent calls compute
deltas correctly; narrow exception handling around fStateSystem.querySingleState
to catch StateSystemDisposedException and the specific state-system query
exceptions rather than Exception, and only continue on those specific
recoverable exceptions while letting other exceptions bubble.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java`:
- Around line 73-74: Replace raw console prints in ProcessFlowInfo with Eclipse
logging: remove System.err.println("Warning: No target thread specified for
hypervisor tracking") and any System.out.printf occurrences and instead call
Activator.getInstance().logWarning(...) for warnings and
Activator.getInstance().logInfo(...) for informational messages; update the code
paths in the ProcessFlowInfo class where those prints occur (the warning branch
and the two printf locations) to construct a clear log message and pass it to
the appropriate Activator logging method.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java`:
- Around line 206-207: The current early-return that skips KVM transitions when
the local vcpuid (vcpuid) doesn't match vmState.getCurrentVirtualCpu() causes
the tracked vCPU to become stale; update VMNativeCallStackStateProvider so that
when processing accepted guest events (the code path around the KVM transition
handling), you refresh the cached vCPU by setting the vmState's current vCPU to
the vCPU observed on the guest event instead of simply returning — i.e., replace
the simple if (vcpuid == null || vcpuid != this.vmState.getCurrentVirtualCpu())
{ return; } logic with code that updates
vmState.getCurrentVirtualCpu()/setCurrentVirtualCpu (or equivalent setter) from
the guest event vCPU before continuing to process host-overhead/KVM transition
events so future host events aren’t filtered out.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Line 362: In VMNativeStateProvider, change the misspelled state attribute
passed to updateFlowStateSystem from "natif_flow" to "native_flow" (search for
the call updateFlowStateSystem(ss, "natif_flow", pid, ts, name) and update the
string literal); also scan for any other occurrences of "natif_flow" in the
class and update them to "native_flow" to keep stored attributes consistent.
- Around line 319-321: Replace the direct System.err.println calls in
VMNativeStateProvider (the catch blocks that currently print "Error handling
workload event..." and the other System.err usage) with the Eclipse logging
utility used elsewhere in this class: call Activator.getInstance().logError(...)
and pass a clear message plus the caught Exception so the stacktrace is
recorded; ensure you update both occurrences and follow the same logging pattern
as the existing Activator.getInstance().logError(...) usage in this class.
---
Duplicate comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java`:
- Around line 268-271: Before replacing currentSequence in the VM_ENTRY case,
persist the existing sequence: if currentSequence is non-null and contains any
recorded guest/host interval data, add it to the collection that stores
completed sequences (e.g., call the method or list that collects
ExecutionSequence instances) before creating a new ExecutionSequence and calling
currentSequence.setVmEntry(flowEvent); ensure you do not add empty/uninitialized
sequences.
---
Nitpick comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java`:
- Around line 191-193: The try/catch in KvmExitRateDataProvider that swallows
IndexOutOfBoundsException should be changed to use the safe lookup API and/or
log failures: replace the throwing quark lookup with optQuarkRelative(...) and
check for the INVALID_ATTRIBUTE sentinel (or equivalent constant) instead of
catching IndexOutOfBoundsException; if you must keep the catch, log the failure
at debug level including the attribute name/quark and context. Locate the quark
lookup call wrapped by the catch (the IndexOutOfBoundsException handler) and
update it to use optQuarkRelative and a conditional skip when it returns
INVALID_ATTRIBUTE, or add a debug processLogger.debug(...) inside the catch to
record the failing lookup.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java`:
- Around line 176-178: In KvmExitStateProvider change the unsafe downcast of
Long to int: currently ((Long) obj).intValue() can silently truncate large
values; replace it with a safe conversion (e.g. Math.toIntExact((Long) obj) or
explicit bounds checks against Integer.MIN_VALUE/Integer.MAX_VALUE) so the code
throws on overflow instead of producing incorrect IDs; update the branch
handling in the method where this cast occurs to use the chosen safe conversion
and handle the resulting ArithmeticException or out-of-range condition
appropriately.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java`:
- Around line 291-296: The code currently treats NATIVE events the same as
hypervisor events by calling ExecutionSequence.addHypervisorEvent(flowEvent)
inside the NATIVE case, which is misleading; change ExecutionSequence to hold
native events separately (e.g., add a new List nativeEvents and a method
addNativeEvent(ITmfEvent or FlowEventType)) and update the NATIVE branch in
ProcessFlowInfo to call currentSequence.addNativeEvent(flowEvent) instead of
addHypervisorEvent; also update any consumers of ExecutionSequence to handle the
new nativeEvents collection where appropriate and add unit/tests for the new
method to preserve behavior separation.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.java`:
- Around line 30-41: ThreadFlowInfo currently exposes core state via
package-visible fields tid, processName, and events and allows external mutation
of events; make tid, processName, and events private, add public getters
getTid() and getProcessName(), replace direct external mutation with a
controlled mutator (e.g., addEvent(KernelEventInfo) or
addEvents(Collection<KernelEventInfo>) implemented on ThreadFlowInfo), and
change the events accessor to return an unmodifiable view
(Collections.unmodifiableList(events)); update any callers that currently read
or write the package-visible fields to use the new getters/mutators.
- Around line 72-77: The ThreadFlowInfo.printFlow method currently uses
System.out.printf to emit unbounded event data; replace these direct stdout
prints with the project logging mechanism (e.g., use the plugin logger/Trace
Compass logging utilities) or accumulate output into a bounded String and log at
a debug/trace level to avoid flooding the console. Specifically, update
printFlow (class ThreadFlowInfo) to stop calling System.out.printf for "Thread
%d (%d events)" and the per-event line that prints evt.timestamp and evt.name;
instead format the message into a single buffer (or use the platform logger) and
emit via the project's logger at an appropriate level so output can be
routed/controlled by the UI/log settings. Ensure you reference fields tid and
events and KernelEventInfo evt when building the message and do not retain any
System.out usage.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java`:
- Around line 35-37: Remove the no-op override of setTrace(ITmfTrace) in class
VMNativeCallStackAnalysis: delete the three-line method that simply returns
super.setTrace(trace) so the class inherits the superclass implementation
directly, leaving no behavioral change but reducing maintenance noise; ensure
there are no local usages that relied on this override before committing.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java`:
- Around line 139-143: The conditional branch in VMNativeCallStackStateProvider
that checks "if (pid == nativeStart.pid)" is redundant because both branches
return the same value; remove the if/else and replace with a single return
combining getProcessName(event) and Long.toString(tid) (the expression currently
returned in both branches), leaving pid, nativeStart.pid, getProcessName(event)
and tid references intact.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Around line 511-513: The VMExecutionState.isInGuest() method is unused; either
remove the method from the VMExecutionState class or, if it is intentionally
kept for future use, annotate it with an appropriate suppression and comment
(e.g., add `@SuppressWarnings`("unused") and a brief rationale) so the
unused-method warning is explicit; update VMNativeStateProvider/VMExecutionState
accordingly to avoid leaving dead code.
- Around line 474-477: The TraceContext inner class currently ignores its
constructor parameters (ITmfTrace trace, TraceType type); either persist them as
fields or eliminate the class if unused—add private final fields (e.g.,
ITmfTrace trace; TraceType type) to VMNativeStateProvider.TraceContext and
assign them in the TraceContext(ITmfTrace trace, TraceType type) constructor
(and add getters if other code needs access), or remove the TraceContext type
and update callers to use the trace and type directly.
- Line 13: Replace the wildcard import in VMNativeStateProvider by listing only
the java.util types actually referenced in that class; remove "import
java.util.*;" and add explicit imports such as java.util.List, java.util.Map,
java.util.Set, java.util.HashMap, java.util.ArrayList, java.util.Collections,
java.util.EnumSet, java.util.Optional, java.util.Iterator, java.util.Objects
(only include the exact types used by VMNativeStateProvider) to avoid wildcard
imports and potential name conflicts.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java`:
- Around line 44-55: TRACE_LAYOUTS is an unused write-only global cache: remove
the TRACE_LAYOUTS field and the TRACE_LAYOUTS.put(trace, layout) call from
canMatchTrace, and simplify canMatchTrace to just check the trace type and
obtain the layout if needed locally (or omit obtaining the layout entirely) so
there is no global mutable state; update references to
IKernelAnalysisEventLayout, TRACE_LAYOUTS.put, and canMatchTrace accordingly.
- Around line 36-42: The set INITIALIZATION in VmSyncLttngEventMatching uses
hard-coded event ID literals (e.g., "vmsync_hg_host", "vmsync_hg_guest",
"vmsync_gh_host", "vmsync_gh_guest"); replace these duplicated string literals
with the canonical constants from QemuKvmStrings (use the corresponding
QemuKvmStrings.VMSYNC_* constants) wherever they appear (both in the static
REQUIRED_EVENTS initialization and the other duplicated block around lines
72–81) so the file references the single source of truth for those event IDs.
- Around line 62-64: The code currently mutates the set returned by
TmfEventTypeCollectionHelper.getEventNames by calling
traceEvents.retainAll(REQUIRED_EVENTS); instead of mutating, perform a read-only
presence check (e.g., use Collections.disjoint(traceEvents, REQUIRED_EVENTS) or
traceEvents.stream().anyMatch(REQUIRED_EVENTS::contains)) and return the boolean
result; update the logic around the traceEvents variable in
VmSyncLttngEventMatching to use that non-mutating check so the helper's returned
set is not modified.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.java`:
- Line 18: EventDensityTreeViewer imports
org.eclipse.tracecompass.internal.tmf.core.histogram.Messages which ties the
class to internal APIs; replace that dependency by adding local message
constants or switching to the public API equivalents and update all usages in
EventDensityTreeViewer to reference the new local constants (or the public API
class) instead of Messages so the code no longer imports the internal
org.eclipse.tracecompass.internal package.
- Around line 36-38: The unchecked cast warning occurs when casting element to
TmfGenericTreeEntry<TmfTreeDataModel> in EventDensityTreeViewer; suppress it by
adding `@SuppressWarnings`("unchecked") on the smallest scope (the method or the
local variable) and add a brief comment that the suppression is safe because you
first check element instanceof TmfGenericTreeEntry and the tree entry is known
to contain TmfTreeDataModel instances, or alternatively replace the cast with a
defensive helper that verifies the generic payload type before casting.
🪄 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: b7f28a40-d352-446c-b7e8-413a0f4a8596
📒 Files selected for processing (25)
analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java
✅ Files skipped from review due to trivial changes (4)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.java
🚧 Files skipped from review as they are similar to previous changes (7)
- analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
.../virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java
Show resolved
Hide resolved
...ecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java
Outdated
Show resolved
Hide resolved
...tor/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Outdated
Show resolved
Hide resolved
beliosien
force-pushed
the
vm-flow-analysis
branch
from
7844f25 to
6a6dafb
Compare
There was a problem hiding this comment.
Actionable comments posted: 7
♻️ Duplicate comments (2)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java (1)
266-269:⚠️ Potential issue | 🔴 CriticalFlush the current sequence before opening the next
VM_ENTRY.A second
VM_ENTRYoverwritescurrentSequence, so every earlier guest/host slice is discarded and only the last one survives.Suggested fix
case VM_ENTRY: + if (currentSequence != null) { + sequences.add(currentSequence); + } currentSequence = new ExecutionSequence(); currentSequence.setVmEntry(flowEvent); break;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 266 - 269, When encountering a VM_ENTRY in ProcessFlowInfo, flush the existing currentSequence before overwriting it: if currentSequence is non-null (and contains any guest/host slices or a vmEntry), add it to the container/list of completed sequences (the same collection used elsewhere in this class) or otherwise finalize it before creating a new ExecutionSequence and calling setVmEntry(flowEvent); update the VM_ENTRY case where currentSequence is reassigned so it preserves and stores the previous sequence rather than discarding it.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java (1)
377-385:⚠️ Potential issue | 🟠 MajorOnly publish host events that were actually correlated.
processHypervisorEvents()returns whether the host event was accepted, but the result is ignored here. As written, every host event aftervmStartis written intovm_flow, even when it was rejected as unrelated, out-of-window, wrong-CPU, or duplicate.Suggested fix
EventType type = getEventType(evt); - processHypervisorEvents(flow, type, evt); - updateFlowStateSystem(ss, "vm_flow", this.vmStart.pid, ts, name); //$NON-NLS-1$ + if (processHypervisorEvents(flow, type, evt)) { + updateFlowStateSystem(ss, "vm_flow", this.vmStart.pid, ts, name); //$NON-NLS-1$ + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` around lines 377 - 385, The host-side branch in VMNativeStateProvider currently calls processHypervisorEvents(flow, type, evt) but ignores its boolean result, causing updateFlowStateSystem("vm_flow", this.vmStart.pid, ...) to be written for every host event after vmStart even if processHypervisorEvents rejected it; change the logic to capture the return value of processHypervisorEvents and only call updateFlowStateSystem (and any other publish actions) when that method returns true (i.e., the event was accepted/correlated), keeping the existing checks on source, vmStart, flows, and flow.
🧹 Nitpick comments (3)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java (2)
251-253: Consider adding@Nullableannotation to thevalueparameter.State system values can be null (representing no value or a gap). While the implementation handles null correctly by returning
0L, adding@Nullableimproves null-safety analysis and documentation.📝 Suggested fix
- protected long extractCountFromValue(Object value) { + protected long extractCountFromValue(`@Nullable` Object value) { return (value instanceof Number) ? ((Number) value).longValue() : 0L; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java` around lines 251 - 253, The parameter `value` of AbstractRateDataProvider.extractCountFromValue can be null from the state system but is not annotated; add a nullable annotation to the method parameter (e.g., annotate the `value` parameter of extractCountFromValue(Object value) with `@Nullable`) and import the appropriate Nullable annotation used in the project (org.eclipse.jdt.annotation.Nullable or javax.annotation.Nullable) so static analysis and documentation reflect that null is expected and already handled.
53-54: Incomplete@authorJavadoc tag.The author tag appears to be truncated:
(Generated from DPDK patternis missing a closing parenthesis or the sentence is incomplete.📝 Suggested fix
- * `@author` Francois Belias (Generated from DPDK pattern + * `@author` Francois Belias🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java` around lines 53 - 54, The class Javadoc for AbstractRateDataProvider has an incomplete `@author` tag "(Generated from DPDK pattern"—fix the tag in the Javadoc for AbstractRateDataProvider by completing the sentence or adding the missing closing parenthesis and any missing author name (e.g., "(Generated from DPDK pattern)") so the `@author` line is a valid, well-formed Javadoc tag.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java (1)
57-86: This provider is not wired into the new density-view path.In the provided wiring for this PR, the factory and viewers all use
KvmExitRateDataProvider, notKvmExitDataProvider. If this class is no longer meant to back a view, consider removing it; otherwise switch the factory/view registration to it so fixes here are not dead maintenance.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java` around lines 57 - 86, The KvmExitDataProvider class is not wired into the new density-view path because the factory and viewers use KvmExitRateDataProvider; either remove KvmExitDataProvider if it's dead code or update the wiring to use it: locate the factory and viewer registrations that reference KvmExitRateDataProvider and replace them with KvmExitDataProvider (or rename/alias KvmExitDataProvider to KvmExitRateDataProvider) so the provider used by the view matches the implemented class (refer to KvmExitDataProvider, KvmExitRateDataProvider and the factory/view registration points).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml`:
- Around line 56-64: The extension registers KvmExitAnalysisModule for every
TmfTrace (tracetype applies="true"), causing the KVM-exit view to appear on
unrelated traces; narrow the registration to kernel traces by updating the
<tracetype> element for KvmExitAnalysisModule to target the kernel trace type
(replace applies="true" with the kernel trace type identifier or set the class
to the kernel trace class) so the module
(org.eclipse.tracecompass.incubator.internal.virtual.machine.analysis.core.flow.analysis.KvmExitAnalysisModule)
only applies to kernel/kvm traces.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java`:
- Around line 90-109: The code currently memoizes environmentType in
considerEvent by inspecting event.getTrace().getName(), causing later events
from different traces to be ignored; instead remove or stop using a single
cached environmentType and determine the environment per incoming event or per
Trace instance (e.g., by using a trace-specific property such as
trace.getTypeId()/trace.getTraceType() or a Map<ITrace,EnvironmentType> keyed by
event.getTrace()), then call considerVirtualizedEvent(event, traceName) or
considerNativeEvent(event, traceName) based on that per-trace detection; update
logic in considerEvent and any other places using the environmentType field
(e.g., where environmentType is set around lines 403-406 and 463-465) to avoid
relying on workspace trace name substrings and to handle mixed/multi-trace
inputs correctly.
- Around line 525-542: The trackCloneEvent method only handles
"syscall_exit_clone" and "syscall_exit_clone3" so children created by fork/vfork
are never registered; update VMNativeCallStackStateProvider.trackCloneEvent to
also check for "syscall_exit_fork" and "syscall_exit_vfork", use
getIntField(event, PID) for the parent and getIntField(event, "ret") for the
child PID as done now, and if parentPid/childPid are valid and
processTree.isInTree(parentPid) call the same registration method
(processTree.registerClone(parentPid, childPid) or the appropriate register*
method if a separate API exists) so fork/vfork descendants are added to the
process tree.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Around line 279-281: The VMNativeStateProvider branch uses the native
constructor but never enables trackHypervisor, so VM-exit/hypervisor events
become no-ops; update the VMNativeStateProvider constructor (or initialization
path in VMNativeStateProvider) to set the trackHypervisor flag to true when the
provider is intended to track hypervisor events, ensuring subsequent calls to
addVMTransition(...) and addHypervisorEvent(...) are effective; locate the
constructor/initializer in class VMNativeStateProvider and set trackHypervisor =
true (or call the existing enabling method) for native-mode instances that
should track the hypervisor, and mirror the same fix for the other branch
referenced around lines 302-311.
- Around line 127-146: The current finalizeFlow checks use this.nativeEnd /
this.vmEnd which are assigned later in handleWorkloadEvent(), so the end-marker
event never triggers finalizeFlow; modify handleWorkloadEvent() so that
immediately after assigning nativeEnd or vmEnd (when detecting the workload end
event) you retrieve the pid via getIntField(event, PID), look up
flows.get(source) and the ProcessFlowInfo for that pid, and call
ProcessFlowInfo.finalizeFlow() if present; do the same for both native and vm
end branches (same logic applies to the other end-event handling locations).
- Around line 284-300: The native ProcessFlowInfo is being created only in the
workload_end branch, so events between nativeStart and nativeEnd are dropped;
move (or also create) the flows.computeIfAbsent(...).computeIfAbsent(pid, p ->
new ProcessFlowInfo(phase, procName)) call into the workload_start branch (the
branch guarded by begin_native where nativeStart is set) so that when
EVENT_MARKER for TraceType.NATIVE_KERNEL sets nativeStart the corresponding
ProcessFlowInfo for pid/procName is allocated and subsequent native events can
be recorded (refer to EVENT_MARKER, TraceType.NATIVE_KERNEL, begin_native,
nativeStart, nativeEnd, flows, ProcessFlowInfo, pid, procName).
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java`:
- Around line 57-73: TreeXyViewer declares the "Legend" column but never
installs a TreeLabelProvider, so the Legend column has no image renderer; fix by
wiring the same label provider used by EventDensityTreeViewer (or a dedicated
Legend-aware provider) into TreeXyViewer: in the TreeXyViewer constructor (or by
overriding the label-provider accessor used by AbstractSelectTreeViewer2) call
the appropriate setter/initializer to attach the TreeLabelProvider that returns
the series/color images for the "Legend" column (ensure the provider supplies
images for entries via TmfTreeViewerEntry.getName or the legend column index).
---
Duplicate comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java`:
- Around line 266-269: When encountering a VM_ENTRY in ProcessFlowInfo, flush
the existing currentSequence before overwriting it: if currentSequence is
non-null (and contains any guest/host slices or a vmEntry), add it to the
container/list of completed sequences (the same collection used elsewhere in
this class) or otherwise finalize it before creating a new ExecutionSequence and
calling setVmEntry(flowEvent); update the VM_ENTRY case where currentSequence is
reassigned so it preserves and stores the previous sequence rather than
discarding it.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Around line 377-385: The host-side branch in VMNativeStateProvider currently
calls processHypervisorEvents(flow, type, evt) but ignores its boolean result,
causing updateFlowStateSystem("vm_flow", this.vmStart.pid, ...) to be written
for every host event after vmStart even if processHypervisorEvents rejected it;
change the logic to capture the return value of processHypervisorEvents and only
call updateFlowStateSystem (and any other publish actions) when that method
returns true (i.e., the event was accepted/correlated), keeping the existing
checks on source, vmStart, flows, and flow.
---
Nitpick comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java`:
- Around line 251-253: The parameter `value` of
AbstractRateDataProvider.extractCountFromValue can be null from the state system
but is not annotated; add a nullable annotation to the method parameter (e.g.,
annotate the `value` parameter of extractCountFromValue(Object value) with
`@Nullable`) and import the appropriate Nullable annotation used in the project
(org.eclipse.jdt.annotation.Nullable or javax.annotation.Nullable) so static
analysis and documentation reflect that null is expected and already handled.
- Around line 53-54: The class Javadoc for AbstractRateDataProvider has an
incomplete `@author` tag "(Generated from DPDK pattern"—fix the tag in the Javadoc
for AbstractRateDataProvider by completing the sentence or adding the missing
closing parenthesis and any missing author name (e.g., "(Generated from DPDK
pattern)") so the `@author` line is a valid, well-formed Javadoc tag.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java`:
- Around line 57-86: The KvmExitDataProvider class is not wired into the new
density-view path because the factory and viewers use KvmExitRateDataProvider;
either remove KvmExitDataProvider if it's dead code or update the wiring to use
it: locate the factory and viewer registrations that reference
KvmExitRateDataProvider and replace them with KvmExitDataProvider (or
rename/alias KvmExitDataProvider to KvmExitRateDataProvider) so the provider
used by the view matches the implemented class (refer to KvmExitDataProvider,
KvmExitRateDataProvider and the factory/view registration points).
🪄 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: cf0ee2f2-edf0-4bdb-8c45-9eacba841db0
📒 Files selected for processing (25)
analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java
✅ Files skipped from review due to trivial changes (5)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.java
🚧 Files skipped from review as they are similar to previous changes (8)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.java
- analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xml
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml
Show resolved
Hide resolved
...tor/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java
Show resolved
Hide resolved
...tor/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Outdated
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...pse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java
Show resolved
Hide resolved
arfio
requested changes
Apr 7, 2026
Contributor
arfio
left a comment
arfio
left a comment
There was a problem hiding this comment.
Thank you for the update, here are some more comments 😄
...acecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/StateProvider.java
Outdated
Show resolved
Hide resolved
...ncubator/internal/virtual/machine/analysis/core/flow/analysis/StateSystemAnalysisModule.java
Outdated
Show resolved
Hide resolved
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
...al/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphDataProvider.java
Outdated
Show resolved
Hide resolved
...racecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/package-info.java
Show resolved
Hide resolved
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Show resolved
Hide resolved
...rnal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
Outdated
Show resolved
Hide resolved
| .build(); | ||
|
|
||
| @Override | ||
| public @Nullable ITmfTreeDataProvider<? extends ITmfTreeDataModel> createProvider(ITmfTrace trace) { |
Contributor
There was a problem hiding this comment.
Change to createDataProvider as this methode has been deprecated
Author
There was a problem hiding this comment.
The method createDataProvider is not recognized as an existing method that I can override. maybe it is another name ?
...al/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphDataProvider.java
Outdated
Show resolved
Hide resolved
...al/virtual/machine/analysis/core/flow/analysis/core/data/provider/TimeGraphDataProvider.java
Outdated
Show resolved
Hide resolved
beliosien
force-pushed
the
vm-flow-analysis
branch
from
6a6dafb to
db9b17f
Compare
Author
|
Hello, thanks for the help. Every comment has been addressed except for this one: "Change to createDataProvider as this method has been deprecated." The function is not really present unless I missed something, but I am ready for another pass. Thanks |
There was a problem hiding this comment.
Actionable comments posted: 8
♻️ Duplicate comments (1)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java (1)
131-133: 🛠️ Refactor suggestion | 🟠 MajorResolve the nullness warnings explicitly instead of suppressing them.
createSelectionTimeQuery(...)andITmfStateInterval#getValue()are handled as nullable here, but the locals/helper signature still erase that nullness andbuild()suppresses the warning entirely. Please model the nullable values explicitly and then remove the suppression. Please verify this against the TMF/JDT null annotations in the target platform.Suggested direction
- SelectionTimeQueryFilter filter = FetchParametersUtils.createSelectionTimeQuery(fetchParameters); + `@Nullable` SelectionTimeQueryFilter filter = FetchParametersUtils.createSelectionTimeQuery(fetchParameters); if (filter == null) { return null; } + SelectionTimeQueryFilter queryFilter = filter; - long[] xValues = filter.getTimesRequested(); + long[] xValues = queryFilter.getTimesRequested(); if (xValues.length <= 1) { return Collections.emptyList(); } // Initialize builders for each selected data series - Map<Integer, RateSeriesBuilder> builderByQuark = initBuilders(ss, filter); + Map<Integer, RateSeriesBuilder> builderByQuark = initBuilders(ss, queryFilter);- Object v = iv.getValue(); + `@Nullable` Object v = iv.getValue(); long count = extractCountFromValue(v);- Object value = interval.getValue(); + `@Nullable` Object value = interval.getValue(); long count = extractCountFromValue(value);- protected long extractCountFromValue(Object value) { + protected long extractCountFromValue(`@Nullable` Object value) { return (value instanceof Number) ? ((Number) value).longValue() : 0L; }Also applies to: 174-176, 197-199, 232-259
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java` around lines 131 - 133, The build() method currently suppresses nullness; instead annotate and handle nullable values explicitly: update createSelectionTimeQuery(...) and any helper signatures in AbstractRateDataProvider to return `@Nullable` where they may produce null (and update callers), annotate ITmfStateInterval#getValue() usages as `@Nullable`, add explicit null checks in build() before constructing the YModel (e.g., guard fId/fName/fValues or replace nulls with safe defaults or throw a controlled exception), remove the `@SuppressWarnings`("null") on build(), and verify all annotations against the TMF/JDT nullness annotations so the compiler understands the null contracts; apply the same changes to the other affected blocks (around the lines referenced and the occurrences at 174-176, 197-199, 232-259).
🧹 Nitpick comments (5)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java (2)
635-645: Prefer iterative ancestry walk over recursion for deep trees.Current recursion works, but an iterative loop avoids potential deep-stack risks on large fork hierarchies.
Proposed refactor
- private boolean isDescendant(int pid, int ancestorPid) { - Integer parent = childToParent.get(pid); - if (parent == null) { - return false; - } - - if (parent == ancestorPid) { - return true; - } - - return isDescendant(parent, ancestorPid); - } + private boolean isDescendant(int pid, int ancestorPid) { + Integer current = childToParent.get(pid); + while (current != null) { + if (current == ancestorPid) { + return true; + } + current = childToParent.get(current); + } + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 635 - 645, The isDescendant method uses recursion and can overflow for deep fork trees; refactor VMNativeCallStackStateProvider.isDescendant(int pid, int ancestorPid) to walk parent links iteratively using the childToParent map: start from pid, loop retrieving parent = childToParent.get(current) until null or equal to ancestorPid, returning true if found and false otherwise, eliminating the recursive call to isDescendant(parent, ancestorPid).
137-142: Remove redundant parent/child branch in thread labeling.Both branches return the same value, so the conditional can be collapsed for clarity.
Proposed cleanup
- // Label parent vs child - if (pid == nativeStart.pid) { - return getProcessName(event) + "-" + Long.toString(tid); //$NON-NLS-1$ - } return getProcessName(event) + "-" + Long.toString(tid); //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 137 - 142, The conditional in VMNativeCallStackStateProvider that checks "if (pid == nativeStart.pid)" is redundant because both branches return the same value; simplify the method by removing the if/else and emitting a single return that uses getProcessName(event) + "-" + Long.toString(tid), leaving the existing variables (pid, nativeStart, tid) untouched and preserving any surrounding comments about parent vs child if desired.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java (1)
89-99: AligngetEventKeyfiltering with TMF event matching framework pattern.The current implementation extracts matching keys from any event containing
cntandvm_uidfields without verifying the event type. Per the Trace Compass framework documentation, the canonical pattern forITmfMatchEventDefinition#getEventKeyimplementations includes event-name validation before field extraction—both for correctness (avoiding accidental cross-event matches) and to align with how synchronization matchers likeTwoWayVmEventMatchingimplement the interface.Add a guard using the event name before field extraction:
Suggested refactor
`@Override` public IEventMatchingKey getEventKey(ITmfEvent event) { + String evname = event.getName(); + if (!REQUIRED_EVENTS.contains(evname)) { + return null; + } + ITmfEventField content = event.getContent(); Long cnt = content.getFieldValue(Long.class, "cnt"); //$NON-NLS-1$ Long vmUid = content.getFieldValue(Long.class, "vm_uid"); //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java` around lines 89 - 99, The getEventKey method in VmSyncLttngEventMatching should first validate the ITmfEvent's name before extracting fields to avoid cross-event matches; update getEventKey(ITmfEvent event) to check event.getName() (or compare to the specific expected event name(s) used by this matcher) and return null if it doesn't match, then proceed to obtain the ITmfEventField, extract "cnt" and "vm_uid" and construct the VmSyncEventKey only when the event name matches and both fields are non-null.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java (1)
65-94: Remove or wire up this alternate provider path.In the current PR wiring,
KvmExitDataProviderFactoryalways exposesKvmExitRateDataProvider.ID, and the new viewers request that same ID. That leavesKvmExitDataProvider.IDwith no visible path into the feature and creates a second implementation that can drift independently.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java` around lines 65 - 94, The PR leaves KvmExitDataProvider.ID unused while KvmExitDataProviderFactory always exposes KvmExitRateDataProvider.ID, creating a duplicate provider path; either remove KvmExitDataProvider or wire it into the factory and viewers: update KvmExitDataProviderFactory to register and return KvmExitDataProvider.ID (or both IDs) and adjust any viewer instantiation that currently requests KvmExitRateDataProvider.ID to request KvmExitDataProvider.ID if this is the intended implementation, or consolidate the two classes into a single provider implementation and remove the unused ID constant to prevent drift (refer to KvmExitDataProvider, KvmExitDataProviderFactory, and KvmExitRateDataProvider.ID).vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java (1)
55-58: Reuse the shared density viewers instead of keeping a second copy here.If this view is meant to present the same density UI, the package already has
EventDensityViewerandEventDensityTreeViewerfor this provider. The copy has already drifted:createChartViewer()falls back to a plainTmfFilteredXYChartViewer, so the BAR styling and histogram tooltip fromEventDensityViewerare skipped.♻️ Suggested simplification
`@Override` protected TmfXYChartViewer createChartViewer(Composite parent) { TmfXYChartSettings settings = new TmfXYChartSettings(TITLE, X_AXIS_TITLE, Y_AXIS_TITLE, 1); - return new TmfFilteredXYChartViewer(parent, settings, KvmExitRateDataProvider.ID); + return new EventDensityViewer(parent, settings); } `@Override` protected `@NonNull` TmfViewer createLeftChildViewer(`@Nullable` Composite parent) { - return new TreeXyViewer(Objects.requireNonNull(parent)); + return new EventDensityTreeViewer(Objects.requireNonNull(parent)); }After that,
TreeXyViewerand the now-unused imports can be dropped.Also applies to: 60-90
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java` around lines 55 - 58, Replace the custom chart/tree viewers with the shared density viewers: in createChartViewer() return an EventDensityViewer configured with the same TmfXYChartSettings and KvmExitRateDataProvider.ID (instead of TmfFilteredXYChartViewer), and likewise use EventDensityTreeViewer for the tree-based viewer implementations referenced around the 60-90 region; remove references to TreeXyViewer and any now-unused imports after switching to EventDensityViewer/EventDensityTreeViewer so the BAR styling and histogram tooltip from EventDensityViewer are preserved.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java`:
- Around line 98-104: The code in KvmExitRateDataProvider.getTree() currently
returns a cached model from fCached even if the state system is still being
built, causing partial topology snapshots to be cached; change the caching logic
so you only return or set fCached after the underlying state system has finished
building—i.e., check the state system completion flag (use the provider's state
system completion API, e.g., stateSystem.isCompleted() or the equivalent method
on the state system instance used by KvmExitRateDataProvider) before reading
from or assigning fCached.getModel(), and skip/refresh the cache while the state
system is incomplete so later VCPUs/exit_reasons are included; apply the same
guard where caching occurs around lines referenced (also at the later caching
site around 153-155).
- Around line 132-141: The exit-reason leaf TmfTreeDataModel nodes are being
created with hasRowModel=false which prevents selection despite backing
RateSeriesBuilder data; in KvmExitRateDataProvider locate the loop that builds
reason nodes (the code that calls new TmfTreeDataModel(reasonNodeId, vcpuNodeId,
Collections.singletonList(reasonName), false, null)) and change the hasRowModel
flag to true so these exit_reasons/* nodes are marked as data-bearing and become
selectable (leave the rest of the constructor args intact).
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java`:
- Around line 151-159: The exit counters are stored as int and will overflow;
update incrementExitCounter to use long/Long for cumulative totals: change
newValue to a long (e.g., long newValue = 1L), treat currentValue as a Long when
reading (if currentValue instanceof Long) and add 1L, and handle legacy Integer
values by converting them to long if encountered; then pass the long (boxed
Long) into ss.modifyAttribute(timestamp, newValue, quark). Update any related
local variables/types inside incrementExitCounter (and any callers if necessary)
to use long/Long so kvm_exits and exit_reasons/* remain monotonic.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java`:
- Around line 274-325: buildExecutionSequences currently starts a new
ExecutionSequence only on VM_ENTRY and keeps sequences in a local list, losing
the preceding guest/VM_EXIT events and never exposing the results; change the
loop so a sequence is started when you encounter the first GUEST_EVENT, VM_EXIT
or NATIVE (i.e., when currentSequence is null and you see those types) and
closed/added to the collection when you see VM_ENTRY (setVmExit/setVmEntry
appropriately), ensure HYPERVISOR_EVENT and NATIVE go into the current
ExecutionSequence via addHypervisorEvent, and finally persist the built
collection (assign the local sequences to the class-level field used by
finalizeFlow or otherwise expose it so finalizeFlow() can consume the sequences)
while keeping the existing handling of VM_EXIT and VM_ENTRY for
setVmExit/setVmEntry.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java`:
- Line 531: In VMNativeCallStackStateProvider (locate the line with
eventName.equals("syscall_exit_vfork")), fix the malformed NLS suppression by
replacing the trailing comment marker `//$NON-NLS-1${` with the correct
`//$NON-NLS-1$` (remove the stray `{`), so tooling recognizes the suppression
correctly.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Around line 113-135: isWorkloadEvent currently returns true for events whose
name startsWith WORKLOAD_UST_PROVIDER + ":" which routes them into the workload
no-op path while handleWorkloadEvent only handles syscall_entry_openat markers,
causing UST start/end markers to be ignored; update isWorkloadEvent (and the
related logic that classifies events) to either exclude
WORKLOAD_UST_PROVIDER-prefixed events or extend handleWorkloadEvent to interpret
the WORKLOAD_UST_PROVIDER format (parse provider:name or check for start/end
markers) so UST start/end markers open/close capture windows; adjust both
isWorkloadEvent and handleWorkloadEvent (and similar logic in the duplicated
region around the other block at 227-276) to use a consistent classification for
EVENT_MARKER vs WORKLOAD_UST_PROVIDER entries.
- Around line 335-343: The code is updating the state system for VM host events
even when processHypervisorEvents(...) rejects them; change the block inside the
VM_HOST branch to capture the boolean result of processHypervisorEvents(flow,
type, evt) and only call updateFlowStateSystem(ss, "vm_flow", this.vmStart.pid,
ts, name) when that result is true (use the existing flow, evt,
getEventType(evt), ss and this.vmStart.pid symbols to locate and modify the
logic).
- Around line 243-275: End-marker branches call ProcessFlowInfo.finalizeFlow()
but do not clear the corresponding start markers, so subsequent events keep
mutating a finalized flow; after the finalizeFlow() call in both end-marker
branches, clear the relevant start marker (set nativeStart = null in the native
end-marker branch and vmStart = null in the VM end-marker branch) so
handleKernelEvent() no longer treats the flow as open.
---
Duplicate comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.java`:
- Around line 131-133: The build() method currently suppresses nullness; instead
annotate and handle nullable values explicitly: update
createSelectionTimeQuery(...) and any helper signatures in
AbstractRateDataProvider to return `@Nullable` where they may produce null (and
update callers), annotate ITmfStateInterval#getValue() usages as `@Nullable`, add
explicit null checks in build() before constructing the YModel (e.g., guard
fId/fName/fValues or replace nulls with safe defaults or throw a controlled
exception), remove the `@SuppressWarnings`("null") on build(), and verify all
annotations against the TMF/JDT nullness annotations so the compiler understands
the null contracts; apply the same changes to the other affected blocks (around
the lines referenced and the occurrences at 174-176, 197-199, 232-259).
---
Nitpick comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java`:
- Around line 65-94: The PR leaves KvmExitDataProvider.ID unused while
KvmExitDataProviderFactory always exposes KvmExitRateDataProvider.ID, creating a
duplicate provider path; either remove KvmExitDataProvider or wire it into the
factory and viewers: update KvmExitDataProviderFactory to register and return
KvmExitDataProvider.ID (or both IDs) and adjust any viewer instantiation that
currently requests KvmExitRateDataProvider.ID to request KvmExitDataProvider.ID
if this is the intended implementation, or consolidate the two classes into a
single provider implementation and remove the unused ID constant to prevent
drift (refer to KvmExitDataProvider, KvmExitDataProviderFactory, and
KvmExitRateDataProvider.ID).
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java`:
- Around line 635-645: The isDescendant method uses recursion and can overflow
for deep fork trees; refactor VMNativeCallStackStateProvider.isDescendant(int
pid, int ancestorPid) to walk parent links iteratively using the childToParent
map: start from pid, loop retrieving parent = childToParent.get(current) until
null or equal to ancestorPid, returning true if found and false otherwise,
eliminating the recursive call to isDescendant(parent, ancestorPid).
- Around line 137-142: The conditional in VMNativeCallStackStateProvider that
checks "if (pid == nativeStart.pid)" is redundant because both branches return
the same value; simplify the method by removing the if/else and emitting a
single return that uses getProcessName(event) + "-" + Long.toString(tid),
leaving the existing variables (pid, nativeStart, tid) untouched and preserving
any surrounding comments about parent vs child if desired.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java`:
- Around line 89-99: The getEventKey method in VmSyncLttngEventMatching should
first validate the ITmfEvent's name before extracting fields to avoid
cross-event matches; update getEventKey(ITmfEvent event) to check
event.getName() (or compare to the specific expected event name(s) used by this
matcher) and return null if it doesn't match, then proceed to obtain the
ITmfEventField, extract "cnt" and "vm_uid" and construct the VmSyncEventKey only
when the event name matches and both fields are non-null.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.java`:
- Around line 55-58: Replace the custom chart/tree viewers with the shared
density viewers: in createChartViewer() return an EventDensityViewer configured
with the same TmfXYChartSettings and KvmExitRateDataProvider.ID (instead of
TmfFilteredXYChartViewer), and likewise use EventDensityTreeViewer for the
tree-based viewer implementations referenced around the 60-90 region; remove
references to TreeXyViewer and any now-unused imports after switching to
EventDensityViewer/EventDensityTreeViewer so the BAR styling and histogram
tooltip from EventDensityViewer are preserved.
🪄 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: 04c18bf6-fb17-4f4d-8f58-22ff7adc0910
📒 Files selected for processing (27)
analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/package-info.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/package-info.java
✅ Files skipped from review due to trivial changes (4)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/package-info.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.java
- analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/package-info.java
🚧 Files skipped from review as they are similar to previous changes (8)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml
.../virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java
Show resolved
Hide resolved
.../virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java
Outdated
Show resolved
Hide resolved
...ass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java
Show resolved
Hide resolved
...ecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java
Show resolved
Hide resolved
...tor/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java
Outdated
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Outdated
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
beliosien
force-pushed
the
vm-flow-analysis
branch
from
db9b17f to
baf5b1d
Compare
There was a problem hiding this comment.
Actionable comments posted: 6
♻️ Duplicate comments (1)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java (1)
328-334:⚠️ Potential issue | 🟡 MinorDon't synthesize a zero baseline on initialization failures.
This still turns every exception into
0L, which can create an artificial first spike for that CPU in the aggregated series. Please keep the failure handling narrow here and skip the CPU instead of manufacturing a counter reset.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java` around lines 328 - 334, In KvmExitRateDataProvider, remove the blanket fPrevCpuCounts.put(cpuId, 0L) in the catch block for the querySingleState/extractCountFromValue sequence; instead, narrow the catch to only the specific expected exceptions (e.g., state system query exceptions or null checks) and on any failure simply skip initializing that cpuId (do not insert a 0L) so the CPU is left out of fPrevCpuCounts and won't produce an artificial spike; optionally log the error with context (cpuId/timestamp) but do not synthesize a baseline value.
🧹 Nitpick comments (11)
vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java (2)
56-66: Consider removing unnecessary empty overrides.Based on the parallel implementation in
UfCallstackAnalysis(from context snippet),InstrumentedCallStackAnalysislikely provides reasonable defaults forgetAnalysisRequirements()andgetDependentAnalyses(). These overrides returning empty collections may be unnecessary.♻️ Proposed simplification
- - `@Override` - public Iterable<TmfAbstractAnalysisRequirement> getAnalysisRequirements() { - // Return empty for now, can be customized later - return Collections.emptyList(); - } - - `@Override` - protected Iterable<IAnalysisModule> getDependentAnalyses() { - // No dependent analyses for now - return Collections.emptyList(); - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java` around lines 56 - 66, Remove the redundant empty overrides in VMNativeCallStackAnalysis: delete the getAnalysisRequirements() and getDependentAnalyses() methods (or replace their bodies with super.getAnalysisRequirements()/super.getDependentAnalyses()) so the class uses the InstrumentedCallStackAnalysis defaults; refer to the VMNativeCallStackAnalysis class and the two methods getAnalysisRequirements() and getDependentAnalyses() when making the change.
45-48: Remove redundantsetTraceoverride.This method only delegates to the superclass without adding any logic. It can be safely removed.
♻️ Proposed removal
- `@Override` - public boolean setTrace(ITmfTrace trace) throws TmfAnalysisException { - return super.setTrace(trace); - } -🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java` around lines 45 - 48, The override of setTrace in VMNativeCallStackAnalysis is redundant because it simply delegates to super.setTrace(trace) without adding behavior; remove the entire setTrace(ITmfTrace trace) method from the VMNativeCallStackAnalysis class so the inherited implementation is used instead, and run a quick compile to ensure no references depend on this override.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java (4)
630-639: Fix inconsistent indentation.The closing brace and return statement have inconsistent indentation levels.
♻️ Proposed fix
private boolean isDescendant(int pid, int ancestorPid) { Integer current = childToParent.get(pid); while (current != null) { if (current == ancestorPid) { return true; } current = childToParent.get(current); - } - return false; - } + } + return false; + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 630 - 639, The method isDescendant has inconsistent indentation for the closing brace and return statement; open VMNativeCallStackStateProvider and in the isDescendant(int pid, int ancestorPid) method (which uses childToParent.get(...)) align the while-loop body, the closing brace that ends the while loop, and the subsequent return false; to the same indentation level as the method body so the braces and return are consistent and readable.
320-324: Remove redundant null check aftertoString().
procNameis obtained fromcommField.toString()which never returnsnull. The null check on line 322 is unnecessary.♻️ Proposed fix
String procName = commField.toString(); - if (procName == null) { - return null; - } - Pattern p = Pattern.compile("CPU (\\d+)/KVM"); //$NON-NLS-1$🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 320 - 324, Remove the redundant null check for procName after calling commField.toString(): in VMNativeCallStackStateProvider (the method where procName is assigned from commField.toString()) delete the "if (procName == null) { return null; }" branch because toString() never returns null and the check is unnecessary; ensure any dependent control flow continues to use procName directly and run existing null-safety assumptions on commField itself if needed.
362-374: Remove duplicate helper methodgetProcessName_.This method duplicates logic from
getProcessName(ITmfEvent)at lines 487-513. Consider reusing the existing method or consolidating the logic.♻️ Proposed consolidation
Replace usages of
getProcessName_(event)withgetProcessName(event)and remove this method:- private static String getProcessName_(ITmfEvent event) { - - Object commField = event.getContent().getField(PROCESS_NAME); - if (commField == null) { - return "unknown"; //$NON-NLS-1$ - } - String value = commField.toString(); - String[] words = value.split("="); //$NON-NLS-1$ - if (words.length != 2) { - return "unknown"; //$NON-NLS-1$ - } - return words[1]; - }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 362 - 374, Duplicate helper getProcessName_ replicates logic from getProcessName; remove getProcessName_ and update all callers to call getProcessName(ITmfEvent) instead. Locate the getProcessName_ method and any references to it (calls within VMNativeCallStackStateProvider) and replace those calls with getProcessName(event), then delete the getProcessName_ method to avoid duplicated logic and keep a single canonical implementation.
611-613: Use Java naming convention for method name.Method names should use camelCase.
SetRootPidshould besetRootPid.♻️ Proposed fix
- void SetRootPid(int pid) { + void setRootPid(int pid) { this.rootPid = pid; }Also update the call site at line 255:
- processTree.SetRootPid(pid); + processTree.setRootPid(pid);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java` around lines 611 - 613, Rename the method SetRootPid to follow Java camelCase by changing its name to setRootPid and keep the behavior (assigning this.rootPid). Update all call sites that invoke SetRootPid to call setRootPid instead so references compile (search for usages of SetRootPid and replace them). Ensure method visibility and signature remain the same except for the renamed identifier.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.java (1)
93-104: Consider adding null checks for required parameters.The constructor accepts nullable types (
String,TraceType) but doesn't validate that required fields likename,processName, andsourceare non-null. This could lead toNullPointerExceptions later in the flow analysis.🛡️ Proposed validation
public KernelEventInfo(String name, long timestamp, int pid, int tid, String processName, TraceType source, int cpuid, int vcpuid, String exitReason) { + this.name = java.util.Objects.requireNonNull(name, "name cannot be null"); - this.name = name; this.timestamp = timestamp; this.pid = pid; this.tid = tid; + this.processName = java.util.Objects.requireNonNull(processName, "processName cannot be null"); - this.processName = processName; + this.source = java.util.Objects.requireNonNull(source, "source cannot be null"); - this.source = source; this.cpuid = cpuid; this.vcpuid = vcpuid; this.exitReason = exitReason; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.java` around lines 93 - 104, The KernelEventInfo constructor currently assigns parameters without validation; add null checks for required parameters (at least name, processName, and source) in the KernelEventInfo(String name, long timestamp, int pid, int tid, String processName, TraceType source, int cpuid, int vcpuid, String exitReason) constructor and throw an appropriate runtime exception (e.g., IllegalArgumentException or use Objects.requireNonNull) when any required argument is null so downstream code cannot receive null values for name, processName, or source.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java (3)
249-251: Use Java naming convention for method name.Method names should use camelCase.
SetTargetVcpuIdshould besetTargetVcpuId.♻️ Proposed fix
- public void SetTargetVcpuId(int id) { + public void setTargetVcpuId(int id) { targetVcpuId = id; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 249 - 251, Method name SetTargetVcpuId in class ProcessFlowInfo violates Java naming conventions; rename the method to setTargetVcpuId and update all internal references/call sites to use setTargetVcpuId (preserve the same parameter type and visibility/signature) so behavior doesn't change; search for usages of SetTargetVcpuId and replace them to avoid compilation errors.
30-32: Add explicit access modifiers to fields.These fields lack explicit access modifiers. For clarity and maintainability, explicitly declare them as package-private or private with getters.
♻️ Proposed fix
- final String phase; - final String processName; - final Map<Integer, ThreadFlowInfo> threadsByTid = new HashMap<>(); + private final String phase; + private final String processName; + private final Map<Integer, ThreadFlowInfo> threadsByTid = new HashMap<>();🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 30 - 32, The fields phase, processName, and threadsByTid in class ProcessFlowInfo are declared without explicit access modifiers; update their declarations to include explicit access (either mark them private and add appropriate getters, or mark them package-private with a comment) to improve encapsulation and clarity—specifically modify the fields phase, processName, and threadsByTid in ProcessFlowInfo and, if you choose private, add corresponding getter methods (e.g., getPhase(), getProcessName(), getThreadsByTid()) and update any callers to use those getters.
47-57: Add explicit access modifiers to constructors.The constructors lack explicit access modifiers. They appear to be intended for package-private access based on usage patterns.
♻️ Proposed fix
// Constructor for non-virtualized environments - ProcessFlowInfo(String phase, String processName) { + /* package */ ProcessFlowInfo(String phase, String processName) { this(phase, processName, false, null); } // Constructor for tracking a specific thread in virtualized environment - ProcessFlowInfo(String phase, String processName, boolean trackHypervisor, Integer targetThreadId) { + /* package */ ProcessFlowInfo(String phase, String processName, boolean trackHypervisor, Integer targetThreadId) { this.phase = phase;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java` around lines 47 - 57, The two constructors ProcessFlowInfo(String, String) and ProcessFlowInfo(String, String, boolean, Integer) should be explicitly documented as package-private: keep their current (no modifier) visibility but add a short comment like "/* package-private */" or Javadoc "@noinstantiate / package-private" immediately above each constructor to indicate the intended package-private access; Do not change the modifiers to public/protected/private—just make the package-private intent explicit next to the constructors.vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java (1)
13-13: Avoid wildcard imports.Replace the wildcard import with explicit imports for better readability and to avoid potential naming conflicts.
♻️ Proposed fix
-import java.util.*; +import java.util.HashMap; +import java.util.Map; +import java.util.regex.Matcher; +import java.util.regex.Pattern;🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java` at line 13, Replace the wildcard import "import java.util.*;" in VMNativeStateProvider with explicit java.util imports used by that class: e.g., import java.util.List; import java.util.Map; import java.util.Set; import java.util.Collections; (and add any other specific java.util types referenced in VMNativeStateProvider such as ArrayList, HashMap, Optional, etc.). Update the import block to include only these explicit types to avoid the wildcard.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.java`:
- Around line 63-66: getDescriptors currently only returns DESCRIPTOR when
KvmExitAnalysisModule exists on the given trace, which misses experiment traces;
update getDescriptors to also check experiment/aggregate traces by iterating
their constituent traces and returning Collections.singletonList(DESCRIPTOR) if
any sub-trace has a KvmExitAnalysisModule. Locate getDescriptors and use
TmfTraceUtils.getAnalysisModuleOfClass(KvmExitAnalysisModule.class,
KvmExitAnalysisModule.ID) on the main trace and, if that is null and the trace
is an experiment/composite, iterate its child traces (e.g., via
((ITmfTrace)trace).getTraces() or the appropriate experiment API) to detect the
module; otherwise return Collections.emptyList(). Ensure you still reference
DESCRIPTOR when returning the descriptor.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java`:
- Around line 173-177: The getIntegerValue(Object obj) method currently
truncates 64-bit Longs via Long.intValue(), which silently wraps; change it to
detect out-of-range Long values: when obj is a Long, read its longValue(), check
it is between Integer.MIN_VALUE and Integer.MAX_VALUE, and only then return
(int)longValue(); otherwise return null to preserve the safe fallback. Update
the branch in getIntegerValue to perform this range check instead of directly
calling intValue().
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Around line 213-219: In getProcessName, the current split logic can access
words[1] when words.length == 1 and crash; update the check to ensure exactly
two parts before returning words[1] (e.g., if words.length != 2 return
"unknown"). Locate the commField.toString() handling in
VMNativeStateProvider#getProcessName and replace the existing bounds check
(words.length == 0 || words.length > 2) with a strict check for words.length ==
2, returning "unknown" otherwise.
- Around line 140-152: The getExitReason method currently returns null when the
split on exitField doesn't produce the expected two parts but returns
"UNKNOWN_EXIT_REASON" for other failures; change the validation around
exitField.toString().split("=") so any unexpected format returns the same
sentinel string: replace the words.length check (currently allowing 0 or >2 to
return null) with a check that ensures exactly two parts are present and if not
return "UNKNOWN_EXIT_REASON", keeping the existing parsing via
Integer.parseInt(words[1]) and the NumberFormatException catch that also returns
"UNKNOWN_EXIT_REASON"; reference getExitReason, exitField, and ExitReasonMap to
locate the change.
- Around line 192-202: In getIntField, the split result check is wrong and can
lead to ArrayIndexOutOfBounds when words.length == 1; change the guard from
"words.length == 0 || words.length > 2" to "words.length != 2" so you only
access words[1], then proceed to Integer.parseInt(words[1]) inside the existing
try/catch; update the logic referencing the local variable words and the
Integer.parseInt call to ensure only two-part "key=value" strings are parsed and
everything else returns null.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.java`:
- Around line 45-55: The current getColumnImage implementation in
EventDensityTreeViewer hides the legend when a checked TmfGenericTreeEntry has
children (it uses the !genericEntry.hasChildren() guard); remove that children
check so checked data rows always call
getLegendImage(genericEntry.getModel().getId()) (but keep any existing exclusion
for the trace-level/model-id if present), and apply the identical change in
KvmExitDensityView.TreeXyViewer.getColumnImage so parent rows that represent
series still show their legend even when they have child entries.
---
Duplicate comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.java`:
- Around line 328-334: In KvmExitRateDataProvider, remove the blanket
fPrevCpuCounts.put(cpuId, 0L) in the catch block for the
querySingleState/extractCountFromValue sequence; instead, narrow the catch to
only the specific expected exceptions (e.g., state system query exceptions or
null checks) and on any failure simply skip initializing that cpuId (do not
insert a 0L) so the CPU is left out of fPrevCpuCounts and won't produce an
artificial spike; optionally log the error with context (cpuId/timestamp) but do
not synthesize a baseline value.
---
Nitpick comments:
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.java`:
- Around line 93-104: The KernelEventInfo constructor currently assigns
parameters without validation; add null checks for required parameters (at least
name, processName, and source) in the KernelEventInfo(String name, long
timestamp, int pid, int tid, String processName, TraceType source, int cpuid,
int vcpuid, String exitReason) constructor and throw an appropriate runtime
exception (e.g., IllegalArgumentException or use Objects.requireNonNull) when
any required argument is null so downstream code cannot receive null values for
name, processName, or source.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.java`:
- Around line 249-251: Method name SetTargetVcpuId in class ProcessFlowInfo
violates Java naming conventions; rename the method to setTargetVcpuId and
update all internal references/call sites to use setTargetVcpuId (preserve the
same parameter type and visibility/signature) so behavior doesn't change; search
for usages of SetTargetVcpuId and replace them to avoid compilation errors.
- Around line 30-32: The fields phase, processName, and threadsByTid in class
ProcessFlowInfo are declared without explicit access modifiers; update their
declarations to include explicit access (either mark them private and add
appropriate getters, or mark them package-private with a comment) to improve
encapsulation and clarity—specifically modify the fields phase, processName, and
threadsByTid in ProcessFlowInfo and, if you choose private, add corresponding
getter methods (e.g., getPhase(), getProcessName(), getThreadsByTid()) and
update any callers to use those getters.
- Around line 47-57: The two constructors ProcessFlowInfo(String, String) and
ProcessFlowInfo(String, String, boolean, Integer) should be explicitly
documented as package-private: keep their current (no modifier) visibility but
add a short comment like "/* package-private */" or Javadoc "@noinstantiate /
package-private" immediately above each constructor to indicate the intended
package-private access; Do not change the modifiers to
public/protected/private—just make the package-private intent explicit next to
the constructors.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.java`:
- Around line 56-66: Remove the redundant empty overrides in
VMNativeCallStackAnalysis: delete the getAnalysisRequirements() and
getDependentAnalyses() methods (or replace their bodies with
super.getAnalysisRequirements()/super.getDependentAnalyses()) so the class uses
the InstrumentedCallStackAnalysis defaults; refer to the
VMNativeCallStackAnalysis class and the two methods getAnalysisRequirements()
and getDependentAnalyses() when making the change.
- Around line 45-48: The override of setTrace in VMNativeCallStackAnalysis is
redundant because it simply delegates to super.setTrace(trace) without adding
behavior; remove the entire setTrace(ITmfTrace trace) method from the
VMNativeCallStackAnalysis class so the inherited implementation is used instead,
and run a quick compile to ensure no references depend on this override.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.java`:
- Around line 630-639: The method isDescendant has inconsistent indentation for
the closing brace and return statement; open VMNativeCallStackStateProvider and
in the isDescendant(int pid, int ancestorPid) method (which uses
childToParent.get(...)) align the while-loop body, the closing brace that ends
the while loop, and the subsequent return false; to the same indentation level
as the method body so the braces and return are consistent and readable.
- Around line 320-324: Remove the redundant null check for procName after
calling commField.toString(): in VMNativeCallStackStateProvider (the method
where procName is assigned from commField.toString()) delete the "if (procName
== null) { return null; }" branch because toString() never returns null and the
check is unnecessary; ensure any dependent control flow continues to use
procName directly and run existing null-safety assumptions on commField itself
if needed.
- Around line 362-374: Duplicate helper getProcessName_ replicates logic from
getProcessName; remove getProcessName_ and update all callers to call
getProcessName(ITmfEvent) instead. Locate the getProcessName_ method and any
references to it (calls within VMNativeCallStackStateProvider) and replace those
calls with getProcessName(event), then delete the getProcessName_ method to
avoid duplicated logic and keep a single canonical implementation.
- Around line 611-613: Rename the method SetRootPid to follow Java camelCase by
changing its name to setRootPid and keep the behavior (assigning this.rootPid).
Update all call sites that invoke SetRootPid to call setRootPid instead so
references compile (search for usages of SetRootPid and replace them). Ensure
method visibility and signature remain the same except for the renamed
identifier.
In
`@vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java`:
- Line 13: Replace the wildcard import "import java.util.*;" in
VMNativeStateProvider with explicit java.util imports used by that class: e.g.,
import java.util.List; import java.util.Map; import java.util.Set; import
java.util.Collections; (and add any other specific java.util types referenced in
VMNativeStateProvider such as ArrayList, HashMap, Optional, etc.). Update the
import block to include only these explicit types to avoid the wildcard.
🪄 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: 8f8adb45-519c-439f-baa7-5ab15f428c33
📒 Files selected for processing (27)
analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExecutionSequence.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/FlowEvent.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KernelEventInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ProcessFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackAnalysis.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeCallStackStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/AbstractRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitRateDataProvider.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/package-info.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xmlvm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityViewer.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/KvmExitDensityView.javavm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/package-info.java
✅ Files skipped from review due to trivial changes (6)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/package-info.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/src/org/eclipse/tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/package-info.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/TraceType.java
- analyses/org.eclipse.tracecompass.incubator.executioncomparison.core/src/org/eclipse/tracecompass/incubator/internal/executioncomparison/core/DifferentialSeqCallGraphAnalysis.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ThreadFlowInfo.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProvider.java
🚧 Files skipped from review as they are similar to previous changes (6)
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitAnalysisModule.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/ExitReasonMap.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncEventKey.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.ui/plugin.xml
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/src/org/eclipse/tracecompass/incubator/internal/virtual/machine/analysis/core/flow/analysis/VmSyncLttngEventMatching.java
- vm/org.eclipse.tracecompass.incubator.virtual.machine.analysis.core/plugin.xml
...rtual/machine/analysis/core/flow/analysis/core/data/provider/KvmExitDataProviderFactory.java
Show resolved
Hide resolved
...ass/incubator/internal/virtual/machine/analysis/core/flow/analysis/KvmExitStateProvider.java
Outdated
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...ss/incubator/internal/virtual/machine/analysis/core/flow/analysis/VMNativeStateProvider.java
Show resolved
Hide resolved
...tracecompass/incubator/virtual/machine/analysis/ui/flow/analysis/EventDensityTreeViewer.java
Show resolved
Hide resolved
Signed-off-by: philippe <beliasossim@gmail.com>
beliosien
force-pushed
the
vm-flow-analysis
branch
from
baf5b1d to
34de4b5
Compare
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 WM flow analysis view that shows the difference in the execution of the same workload inside a virtual machine and a native system.
How to test
1 - Open TraceCompass
2 - Import the trace under the same experiment
4 - Under VM Native Callstack, select Flame Graph and make sure that the view shows the Flame Graph of each process inside your workload.
5 - Open Execution comparison view
6 - Verify that the view shows boxes with syscalls event inside
Follow-ups
Review checklist
Summary by CodeRabbit
New Features
Chores