Repository navigation
Fetch latest timeline events before exporting Performance data - #10055
Conversation
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that the latest timeline events are pulled from the VM before exporting performance data, preventing recent events from being missing. It introduces fetchFullDataBeforeExport in PerformanceController and pullLatestTimelineEvents in TimelineEventsController, along with corresponding unit tests. A review comment highlights a potential race condition where concurrent calls to _pullPerfettoVmTimeline could result in duplicate timeline events, and suggests tracking the active pull future to prevent overlapping requests.
| Future<void> pullLatestTimelineEvents() async { | ||
| try { | ||
| await _pullPerfettoVmTimeline(); | ||
| } catch (e, st) { | ||
| // Don't block the export of the data we already have. | ||
| _log.warning('Failed to pull the latest timeline events.', e, st); | ||
| } | ||
| } |
There was a problem hiding this comment.
[MUST-FIX] Potential Race Condition / Duplicate Timeline Events
Calling _pullPerfettoVmTimeline() concurrently can lead to a race condition where duplicate timeline events are fetched and added to traceRingBuffer.
Why this happens:
In _pullPerfettoVmTimeline(), _nextPollStartMicros is read at the beginning of the method but is only updated at the very end (after several asynchronous await points like getVMTimelineMicros and getPerfettoVMTimeline).
If pullLatestTimelineEvents() is called while a periodic poll (via _timelinePollingRateLimiter) or a manual refresh (via forceRefresh()) is already in progress, both executions will use the same _nextPollStartMicros value, resulting in overlapping time ranges and duplicate trace packets being appended to traceRingBuffer.
Suggested Fix:
To prevent concurrent executions of _pullPerfettoVmTimeline(), we should track the active pull Future and return it if a pull is already in progress.
Since _pullPerfettoVmTimeline is outside the current diff hunk, you can declare a Future<void>? _activePull field in TimelineEventsController and update _pullPerfettoVmTimeline as follows:
Future<void>? _activePull;
Future<void> _pullPerfettoVmTimeline({bool isInitialPull = false}) async {
if (_activePull != null) {
return _activePull;
}
final completer = Completer<void>();
_activePull = completer.future;
try {
final service = serviceConnection.serviceManager.service;
if (service == null) return;
// ... existing implementation ...
completer.complete();
} catch (e, st) {
completer.completeError(e, st);
rethrow;
} finally {
_activePull = null;
}
}References
- Prefix every comment with a severity: [MUST-FIX] for logical bugs. (link)
…-9731-export-timeline-events # Conflicts: # packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md
Fixes #9731
this PR fixes issue #9731 where data exported from the Performance screen was missing the most recent timeline events. Timeline events are only pulled from the VM every 10 seconds, so frames drawn after the last poll were exported without them. Now the latest timeline events are pulled before exporting, the same way the Network screen does with
fetchFullDataBeforeExport.Before
most recent frames (1331–1411) had no timeline events in the file.
After
all of them had timeline events, including the most recent frame
Pre-launch Checklist
General checklist
///).Issues checklist
contributions-welcomeorgood-first-issuelabel.contributions-welcomeorgood-first-issuelabel. I understand this means my PR might take longer to be reviewed.Tests checklist
AI-tooling checklist
Feature-change checklist
release-notes-not-requiredlabel or left a comment requesting the label be added.packages/devtools_app/release_notes/NEXT_RELEASE_NOTES.md.If you need help, consider asking for help on Discord.