⚡ Bolt: [performance improvement] Parallelize icon export to YAML#413
⚡ Bolt: [performance improvement] Parallelize icon export to YAML#413aafre wants to merge 1 commit into
Conversation
Co-authored-by: aafre <8656674+aafre@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Summary of ChangesHello, 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 a notable performance enhancement to the icon export mechanism within the 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. 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 effectively improves performance by parallelizing icon exports using Promise.all. The change correctly identifies and addresses a bottleneck where I/O-bound tasks were executed sequentially. My review includes one suggestion to further refine the implementation by adopting a more functional approach that avoids mutating a shared object from concurrent callbacks, aligning with best practices for avoiding side effects, which will make the code more robust and easier to maintain.
| const targetFilenames = filenames || Object.keys(registry); | ||
| const exportData: IconExportData = {}; | ||
|
|
||
| // ⚡ Bolt: Parallelized FileReader conversions using Promise.all | ||
| // Why: Sequential `await` in loops for I/O-bound tasks blocks the main thread | ||
| // Impact: Reduces export time from O(N) to roughly O(1) for N icons | ||
| // Measurement: Exporting a resume with 10 icons will now take the time of the slowest single icon conversion instead of the sum of all 10. | ||
| const exportPromises = targetFilenames.map(async (filename) => { | ||
| const entry = registry[filename]; | ||
| if (entry) { | ||
| try { | ||
| const base64Data = await fileToBase64(entry.file); | ||
| exportData[filename] = { | ||
| data: base64Data, | ||
| type: entry.file.type, | ||
| size: entry.file.size, | ||
| uploadedAt: entry.uploadedAt.toISOString(), | ||
| }; | ||
| } catch (error) { | ||
| console.warn(`Failed to export icon ${filename}:`, error); | ||
| } | ||
| } | ||
| }); | ||
|
|
||
| for (const filename of targetFilenames) { | ||
| const entry = registry[filename]; | ||
| if (entry) { | ||
| await Promise.all(exportPromises); | ||
|
|
||
| return exportData; |
There was a problem hiding this comment.
While using Promise.all is a great performance improvement, mutating an external object (exportData) from concurrent async callbacks can be problematic and is generally considered an unsafe pattern. It's better to use a more functional approach where each promise resolves to a value, and then you can collect all the results to build the final object. This avoids side effects and makes the code easier to reason about.
Here's a suggested refactoring that has the map function return key-value pairs, which are then collected into the final object after all promises resolve.
const targetFilenames = filenames || Object.keys(registry);
// ⚡ Bolt: Parallelized FileReader conversions using Promise.all
// Why: Sequential `await` in loops for I/O-bound tasks blocks the main thread
// Impact: Reduces export time from O(N) to roughly O(1) for N icons
// Measurement: Exporting a resume with 10 icons will now take the time of the slowest single icon conversion instead of the sum of all 10.
const exportPromises = targetFilenames.map(
async (filename): Promise<[string, IconExportData[string]] | null> => {
const entry = registry[filename];
if (!entry) {
return null;
}
try {
const base64Data = await fileToBase64(entry.file);
return [
filename,
{
data: base64Data,
type: entry.file.type,
size: entry.file.size,
uploadedAt: entry.uploadedAt.toISOString(),
},
];
} catch (error) {
console.warn(`Failed to export icon ${filename}:`, error);
return null;
}
},
);
const results = await Promise.all(exportPromises);
const validResults = results.filter(
(result): result is [string, IconExportData[string]] => result !== null,
);
return Object.fromEntries(validResults);References
- The principle of avoiding side effects in functions, especially in concurrent operations, is crucial for predictable and maintainable code. Mutating external objects from concurrent callbacks can lead to race conditions and unexpected behavior.
1 participant
💡 What: Refactored the
exportIconsForYAMLfunction inuseIconRegistry.tsto usePromise.allfor parallelizing asynchronousFileReaderbase64 conversions instead of executing them sequentially in afor...ofloop.🎯 Why: I/O-bound tasks like
FileReaderconversions were being awaited sequentially. This blocked the main thread and scaled linearly, meaning that exporting a resume with 10 icons would take the sum of the time required for all 10 conversions.📊 Impact: Reduces total export time from O(N) to roughly O(1). Converting 10 icons now takes roughly the time of the single slowest conversion, significantly speeding up YAML export and persistence operations.
🔬 Measurement: Verify by comparing the execution time of
exportIconsForYAMLbefore and after this change when exporting a registry with a large number of icons. Tests were run inresume-builder-uiviapnpm testanduseIconRegistrytest files executed successfully.PR created automatically by Jules for task 17964273577296936865 started by @aafre