perf(splash): optimize reactive task state management and reduce unnecessary rebuilds#16
Conversation
…cessary rebuilds Replace Set/Map-based state tracking with fixed-size lists for O(1) access. Add counters to cache loading state instead of iterating collections. Minimize setState calls by checking if state actually changed before updating. Cache SplashTaskError string representation. Add early returns in providers when no tasks exist. Use eagerError in Future.wait for faster failure detection. Make oneTimeTasks and reactiveTasks
Summary of ChangesHello @pckimlong, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request delivers significant performance enhancements and stability improvements to the splash screen task management system. It refactors the internal state tracking to use more efficient data structures, reduces redundant UI rebuilds, and optimizes error handling and task execution logic. These changes aim to provide a smoother, faster, and more reliable splash experience for users. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![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 introduces significant performance optimizations to the splash screen task management, primarily by replacing map/set-based state tracking with more efficient fixed-size lists and counters, and by reducing unnecessary widget rebuilds. The changes are well-reasoned and align with the goals outlined in the description. Caching the string representation of SplashTaskError and using eagerError in Future.wait are also smart improvements. The addition of a comprehensive test suite is particularly commendable and greatly increases confidence in the correctness of these optimizations. My feedback includes a few minor suggestions to improve code clarity and maintainability.
| // Only call setState if state actually changed | ||
| if ((shouldMarkChanged && !wasChanged) || wasActive != isNowActive) { | ||
| setState(() { | ||
| if (shouldMarkChanged && !wasChanged) { | ||
| _changedWatchValues[taskIndex] = true; | ||
| _changedWatchCount++; | ||
| } | ||
| if (wasActive != isNowActive) { | ||
| _activeWatchValues[taskIndex] = isNowActive; | ||
| _activeWatchCount += isNowActive ? 1 : -1; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
To improve readability and avoid redundant logic, the conditions for updating the state can be evaluated once and stored in local boolean variables. This makes the code cleaner and easier to follow, as the conditions are not re-evaluated inside the setState block.
// Only call setState if state actually changed
final needsChangeUpdate = shouldMarkChanged && !wasChanged;
final needsActiveUpdate = wasActive != isNowActive;
if (needsChangeUpdate || needsActiveUpdate) {
setState(() {
if (needsChangeUpdate) {
_changedWatchValues[taskIndex] = true;
_changedWatchCount++;
}
if (needsActiveUpdate) {
_activeWatchValues[taskIndex] = isNowActive;
_activeWatchCount += isNowActive ? 1 : -1;
}
});
}| // Only call setState if state actually changed | ||
| if (wasActive != shouldBeActive || shouldClearChanged) { | ||
| setState(() { | ||
| if (wasActive != shouldBeActive) { | ||
| _activeExecuteTasks[taskIndex] = shouldBeActive; | ||
| _activeExecuteCount += shouldBeActive ? 1 : -1; | ||
| } | ||
| if (shouldClearChanged) { | ||
| _changedWatchValues[taskIndex] = false; | ||
| _changedWatchCount--; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
Similar to the watch provider listener, the condition wasActive != shouldBeActive is checked twice. Storing this in a local boolean variable would make the code more readable and prevent re-evaluating the same condition.
// Only call setState if state actually changed
final needsActiveUpdate = wasActive != shouldBeActive;
if (needsActiveUpdate || shouldClearChanged) {
setState(() {
if (needsActiveUpdate) {
_activeExecuteTasks[taskIndex] = shouldBeActive;
_activeExecuteCount += shouldBeActive ? 1 : -1;
}
if (shouldClearChanged) {
_changedWatchValues[taskIndex] = false;
_changedWatchCount--;
}
});
}| if (tasks.isNotEmpty) { | ||
| if (config.runOneTimeTaskInParallel) { | ||
| await Future.wait([for (final task in tasks) task(ref)]); | ||
| // Use more efficient Future.wait with growable: false |
There was a problem hiding this comment.
The comment mentions using growable: false, but a list literal [...] creates a growable list in Dart. While the performance impact is negligible, the comment is inaccurate. It would be clearer to update the comment to reflect the primary optimization here, which is the use of eagerError.
| // Use more efficient Future.wait with growable: false | |
| // Use Future.wait with eagerError for faster failure detection |
| final stackLines = stack.toString().split('\n'); | ||
| final limitedStack = stackLines.length > 5 | ||
| ? stackLines.sublist(0, 5).join('\n') | ||
| : stackLines.join('\n'); |
There was a problem hiding this comment.
The logic to limit the stack trace to the first 5 lines can be simplified. The previous implementation using .take(5) is more concise and idiomatic in Dart for this purpose. It achieves the same result with better readability.
| final stackLines = stack.toString().split('\n'); | |
| final limitedStack = stackLines.length > 5 | |
| ? stackLines.sublist(0, 5).join('\n') | |
| : stackLines.join('\n'); | |
| final limitedStack = stack.toString().split('\n').take(5).join('\n'); |
1 participant
Replace Set/Map-based state tracking with fixed-size lists for O(1) access. Add counters to cache loading state instead of iterating collections. Minimize setState calls by checking if state actually changed before updating. Cache SplashTaskError string representation. Add early returns in providers when no tasks exist. Use eagerError in Future.wait for faster failure detection. Make oneTimeTasks and reactiveTasks