Thread statuses improvements#398
Merged
MatthewKhouzam merged 2 commits intoMay 12, 2026
Merged
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (15)
✅ Files skipped from review due to trivial changes (4)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughTightened timestamp filtering in KernelThreadInformationProvider; refactored FlameChartDataProvider to group multiple linked kernel entry IDs per thread and build kernel states per linked entry; tests and expected fixtures updated for syscall labeling; stub event handling refined; minor annotation formatting edits. ChangesThread Status Interval Filtering
Flame Chart Data Handling
Tests, Tooltip, Stub & Fixtures
Sequence Diagram(s)sequenceDiagram
participant Client
participant FlameChart as FlameChartDataProvider
participant SS as StateSystem
participant Tests
Client->>FlameChart: request kernel states for TidInformation list
FlameChart->>FlameChart: group TidInformation by TID -> Set<linkedEntryIds>
FlameChart->>SS: query kernel/process statuses for each TID once
SS->>FlameChart: return status intervals
FlameChart->>FlameChart: for each linkedEntryId -> resolve entryId, build TimeGraphState, filter, store keyed by linkedEntryId
FlameChart->>Tests: supply states (with syscall labels/styles) for assertions
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3 | ❌ 2❌ Failed checks (1 warning, 1 inconclusive)
✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Tip 💬 Introducing Slack Agent: The best way for teams to turn conversations into code.Slack Agent is built on CodeRabbit's deep understanding of your code, so your team can collaborate across the entire SDLC without losing context.
Built for teams:
One agent for your entire SDLC. Right inside Slack. 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: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@analysis/org.eclipse.tracecompass.analysis.os.linux.core/src/org/eclipse/tracecompass/analysis/os/linux/core/kernel/KernelThreadInformationProvider.java`:
- Line 551: The filter predicate for currentTimes is incorrect: change the
boolean operator so it keeps times within the session window instead of almost
always allowing them; in the times.stream() filter that references
ss.getCurrentEndTime() and ss.getStartTime(), replace the OR condition with an
AND (so times satisfy time < ss.getCurrentEndTime() && time > ss.getStartTime(),
adjust inclusivity if needed) to ensure only timestamps inside the current time
window are collected before sorting and collecting into currentTimes.
In
`@analysis/org.eclipse.tracecompass.analysis.profiling.core/src/org/eclipse/tracecompass/internal/analysis/profiling/core/instrumented/FlameChartDataProvider.java`:
- Around line 583-585: The threadIdsToEntryIds collection currently uses
Collectors.toList(), which allows duplicate tid.fLinked values per thread and
leads to duplicate kernel states during the status-expansion that follows;
change the collector on the tids stream (the code that builds
threadIdsToEntryIds in FlameChartDataProvider) to collect unique linked IDs
instead (e.g., use Collectors.toCollection(LinkedHashSet::new) or
Collectors.toSet() so each tid.fLinked appears only once per thread) and update
any downstream code that expects a List (the expansion loop that iterates over
threadIdsToEntryIds entries) to iterate the resulting Set or convert it to a
List if ordering is required.
🪄 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: e64aa7c9-86b3-4b37-be29-a807f56940e3
📒 Files selected for processing (2)
analysis/org.eclipse.tracecompass.analysis.os.linux.core/src/org/eclipse/tracecompass/analysis/os/linux/core/kernel/KernelThreadInformationProvider.javaanalysis/org.eclipse.tracecompass.analysis.profiling.core/src/org/eclipse/tracecompass/internal/analysis/profiling/core/instrumented/FlameChartDataProvider.java
arfio
force-pushed
the
thread-statuses-improvements
branch
from
bdd20cc to
84539f9
Compare
…he same tid Signed-off-by: Arnaud Fiorini <fiorini.arnaud@gmail.com>
arfio
force-pushed
the
thread-statuses-improvements
branch
from
84539f9 to
9a9e5c5
Compare
There was a problem hiding this comment.
🧹 Nitpick comments (2)
analysis/org.eclipse.tracecompass.analysis.profiling.core.tests/src/org/eclipse/tracecompass/analysis/profiling/core/tests/FlameChartDataProviderTest.java (2)
29-30: 💤 Low valueRemove commented-out duplicate imports.
Lines 29 and 59 are commented-out versions of their immediately adjacent active imports — debug artifacts that add noise.
🧹 Proposed cleanup
-//import java.util.stream.Collectors; import java.util.stream.Collectors;import org.eclipse.tracecompass.tmf.core.model.annotations.Annotation; -//import org.eclipse.tracecompass.tmf.core.model.annotations.Annotation;Also applies to: 58-59
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@analysis/org.eclipse.tracecompass.analysis.profiling.core.tests/src/org/eclipse/tracecompass/analysis/profiling/core/tests/FlameChartDataProviderTest.java` around lines 29 - 30, In FlameChartDataProviderTest remove the commented-out duplicate import statements for java.util.stream.Collectors (the commented line above the active import and the duplicate at the later import block) so only the single active import remains; locate the commented "//import java.util.stream.Collectors;" entries near the top of the class and delete them to clean up noise introduced during editing.
439-444: 💤 Low valueDead-code branch in
verifyStatesafter the newassertEquals.Adding
assertEquals(expectedStates.size(), states.size())at line 439 is the right fix — it catches both under- and over-delivery of states. However, it makes lines 442–444 unreachable: the loop variableiis bounded bystates.size(), which is now asserted equal toexpectedStates.size(), soi > expectedStates.size() - 1is alwaysfalse. Thefail(...)can never execute.🧹 Proposed cleanup
assertEquals(expectedStates.size(), states.size()); for (int i = 0; i < states.size(); i++) { String entryName = entry.getName(); - if (i > expectedStates.size() - 1) { - fail("Unexpected state at position " + i + FOR_ENTRY + entryName + ": " + states.get(i)); - } ITimeGraphState actual = states.get(i); ITimeGraphState expected = expectedStates.get(i);🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@analysis/org.eclipse.tracecompass.analysis.profiling.core.tests/src/org/eclipse/tracecompass/analysis/profiling/core/tests/FlameChartDataProviderTest.java` around lines 439 - 444, The new assertEquals(expectedStates.size(), states.size()) makes the conditional inside verifyStates' loop unreachable; remove the dead branch that checks "if (i > expectedStates.size() - 1) { fail(... entry.getName() ... states.get(i)) }" or replace the loop to iterate over expectedStates.size() and directly compare elements, so the fail(...) call and the unreachable i check are deleted; update verifyStates to perform direct comparisons between expectedStates.get(i) and states.get(i) (using entry.getName() only in failure messages).
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Nitpick comments:
In
`@analysis/org.eclipse.tracecompass.analysis.profiling.core.tests/src/org/eclipse/tracecompass/analysis/profiling/core/tests/FlameChartDataProviderTest.java`:
- Around line 29-30: In FlameChartDataProviderTest remove the commented-out
duplicate import statements for java.util.stream.Collectors (the commented line
above the active import and the duplicate at the later import block) so only the
single active import remains; locate the commented "//import
java.util.stream.Collectors;" entries near the top of the class and delete them
to clean up noise introduced during editing.
- Around line 439-444: The new assertEquals(expectedStates.size(),
states.size()) makes the conditional inside verifyStates' loop unreachable;
remove the dead branch that checks "if (i > expectedStates.size() - 1) {
fail(... entry.getName() ... states.get(i)) }" or replace the loop to iterate
over expectedStates.size() and directly compare elements, so the fail(...) call
and the unreachable i check are deleted; update verifyStates to perform direct
comparisons between expectedStates.get(i) and states.get(i) (using
entry.getName() only in failure messages).
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 7a65ed87-2044-4802-bcab-72b639b00a43
📒 Files selected for processing (4)
analysis/org.eclipse.tracecompass.analysis.os.linux.core/src/org/eclipse/tracecompass/analysis/os/linux/core/kernel/KernelThreadInformationProvider.javaanalysis/org.eclipse.tracecompass.analysis.profiling.core.tests/src/org/eclipse/tracecompass/analysis/profiling/core/tests/FlameChartDataProviderTest.javaanalysis/org.eclipse.tracecompass.analysis.profiling.core.tests/stubs/org/eclipse/tracecompass/analysis/profiling/core/tests/stubs2/KernelStateProviderStub.javaanalysis/org.eclipse.tracecompass.analysis.profiling.core/src/org/eclipse/tracecompass/internal/analysis/profiling/core/instrumented/FlameChartDataProvider.java
🚧 Files skipped from review as they are similar to previous changes (2)
- analysis/org.eclipse.tracecompass.analysis.os.linux.core/src/org/eclipse/tracecompass/analysis/os/linux/core/kernel/KernelThreadInformationProvider.java
- analysis/org.eclipse.tracecompass.analysis.profiling.core/src/org/eclipse/tracecompass/internal/analysis/profiling/core/instrumented/FlameChartDataProvider.java
Signed-off-by: Arnaud Fiorini <fiorini.arnaud@gmail.com>
arfio
force-pushed
the
thread-statuses-improvements
branch
from
9a9e5c5 to
32c7b50
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
How to test
Open a callstack analysis that have threads callstacks with a kernel trace alongside it. For example using eclipse-tracecompass-incubator/org.eclipse.tracecompass.incubator#265
Follow-ups
N/A
Review checklist
Summary by CodeRabbit