Correct units and cumulative bug in GC times#9890
Conversation
Fixes flutter#7080 In the logging page, GC events were displayed with summaries like idle collection in 290 ms. The value 290 was calculated directly from the cumulative times (`newSpace.time` and `oldSpace.time`), meaning it represented the total cumulative GC time since isolate startup rather than the duration of this specific GC. In reality, the individual collection took 290 microseconds (which is 0.29 milliseconds), but since it was displaying the cumulative time of 290 ms, it was off by a factor of 1000x. In this fix, we update `_handleGCEvent` to track and subtract the previous cumulative GC time from the new cumulative GC time to compute the actual duration delta for this specific GC. Also format the duration as a millisecond double with 1 decimal place (consistent with how frame times are displayed), and appended it to the log summary (e.g. "idle collection in 100.0 ms").
![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 updates the LoggingController to track and display the duration delta of GC events per isolate, rather than displaying the total cumulative GC time, and adds corresponding tests. The feedback highlights a critical issue where the non-existent Future.syncValue is used in mock services, which will cause compilation errors. Additionally, there are concerns regarding a potential memory leak due to uncleaned isolate GC tracking maps, and a recommendation to add defensive checks against negative GC durations if cumulative times reset.
| Future<UriList> lookupPackageUris( | ||
| String isolateId, | ||
| List<String> uris, | ||
| ) => Future.syncValue(UriList( | ||
| uris: _resolvedUriMap != null | ||
| ? (uris.map((e) => _resolvedUriMap[e]).toList()) | ||
| : null | ||
| )); |
There was a problem hiding this comment.
[MUST-FIX] Future.syncValue is not a standard Dart Future constructor or method. It should be replaced with Future.value.
| Future<UriList> lookupPackageUris( | |
| String isolateId, | |
| List<String> uris, | |
| ) => Future.syncValue(UriList( | |
| uris: _resolvedUriMap != null | |
| ? (uris.map((e) => _resolvedUriMap[e]).toList()) | |
| : null | |
| )); | |
| Future<UriList> lookupPackageUris( | |
| String isolateId, | |
| List<String> uris, | |
| ) => Future.value(UriList( | |
| uris: _resolvedUriMap != null | |
| ? (uris.map((e) => _resolvedUriMap[e]).toList()) | |
| : null, | |
| )); |
References
- Categorize Severity: Prefix every comment with a severity like [MUST-FIX] for logical bugs. (link)
There was a problem hiding this comment.
syncValue is being recommended over value. It was introduced in Dart 3.10 or so.
| /// Tracks the previous cumulative GC times (in seconds) for each active isolate. | ||
| /// | ||
| /// This is used to compute the actual duration of the current GC event (by taking | ||
| /// the delta between consecutive cumulative times), rather than displaying the | ||
| /// total cumulative GC time. | ||
| final _previousGcTimesByIsolate = <String, double>{}; |
There was a problem hiding this comment.
[CONCERN] The _previousGcTimesByIsolate map tracks cumulative GC times for each active isolate, but entries are never removed when an isolate exits. In applications that spawn and destroy many short-lived isolates, this can lead to a memory leak. Consider cleaning up the map when isolates are destroyed (e.g., by listening to isolate exit events via the IsolateManager).
References
- Categorize Severity: Prefix every comment with a severity like [CONCERN] for maintainability issues. (link)
2 participants
Fixes #7080
In the logging page, GC events were displayed with summaries like idle collection in 290 ms. The value 290 was calculated directly from the cumulative times (
newSpace.timeandoldSpace.time), meaning it represented the total cumulative GC time since isolate startup rather than the duration of this specific GC. In reality, the individual collection took 290 microseconds (which is 0.29 milliseconds), but since it was displaying the cumulative time of 290 ms, it was off by a factor of 1000x.In this fix, we update
_handleGCEventto track and subtract the previous cumulative GC time from the new cumulative GC time to compute the actual duration delta for this specific GC.Also format the duration as a millisecond double with 1 decimal place (consistent with how frame times are displayed), and appended it to the log summary (e.g. "idle collection in 100.0 ms").
List which issues are fixed by this PR.
Replace this paragraph with a description of what this PR is changing or adding, and why. If your PR is updating any UI functionality, please include include before/after screenshots and/or a gif of the UI interaction.