Conversation
- Refactor `natives/native-worker.js` to extract `executeQuery` logic. - Add `queryBatch` command to `natives/native-worker.js` for executing multiple queries. - Update `src/nativeWorker.ts` to use `queryBatch` for fetching pragmas. - Reduced pragma fetching time by ~3.4x (benchmark: 9.76ms -> 2.85ms per batch). Co-authored-by: zknpr <96851588+zknpr@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. |
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Summary of ChangesHello @zknpr, 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 significantly enhances the database initialization process by optimizing how pragma settings are fetched. By introducing a batching mechanism for SQL queries over IPC, it reduces the overhead of multiple individual calls, resulting in a notable speedup in database operations. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Changelog
Activity
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 a significant performance optimization by batching Pragma fetching into a single IPC call. The refactoring in natives/native-worker.js to create a reusable executeQuery function is a great improvement for code structure and maintainability. The implementation of queryBatch is clean and effectively reduces the number of IPC calls. I have a couple of suggestions to enhance type safety and code robustness.
| try { | ||
| const chg = db.prepare("SELECT changes() as c").all()[0].c; | ||
| rowCount = chg; | ||
| } catch(e) { rowCount = 0; } |
There was a problem hiding this comment.
Relying on a try...catch block to handle an empty result set from db.prepare(...).all() can be considered an anti-pattern. It's more robust to explicitly check the array length before accessing an element. This makes the code's intent clearer and avoids using exceptions for control flow in a non-exceptional case.
| try { | |
| const chg = db.prepare("SELECT changes() as c").all()[0].c; | |
| rowCount = chg; | |
| } catch(e) { rowCount = 0; } | |
| try { | |
| const res = db.prepare("SELECT changes() as c").all(); | |
| rowCount = res.length > 0 ? res[0].c : 0; | |
| } catch(e) { rowCount = 0; } |
| const res = await worker.call<any>('queryBatch', [queries]); | ||
|
|
||
| const result: Record<string, CellValue> = {}; | ||
|
|
||
| for (const pragma of pragmasToFetch) { | ||
| const res = await worker.call<any>('query', [`PRAGMA ${pragma}`]); | ||
| if (res && res.values && res.values.length > 0) { | ||
| result[pragma] = res.values[0][0]; | ||
| } | ||
| if (res && res.results && Array.isArray(res.results)) { | ||
| res.results.forEach((r: any, i: number) => { | ||
| if (r && r.values && r.values.length > 0) { | ||
| result[pragmasToFetch[i]] = r.values[0][0]; | ||
| } | ||
| }); | ||
| } |
There was a problem hiding this comment.
To improve type safety and code clarity, it's best to avoid using any. You can define an interface for the queryBatch response and use it to type the result from worker.call. This will provide better autocompletion and compile-time checks.
interface QueryBatchResult {
results: {
values: CellValue[][];
}[];
}
const res = await worker.call<QueryBatchResult>('queryBatch', [queries]);
const result: Record<string, CellValue> = {};
if (res?.results) {
res.results.forEach((r, i) => {
if (r?.values?.length > 0 && r.values[0]?.length > 0) {
result[pragmasToFetch[i]] = r.values[0][0];
}
});
}
|
Merged as part of v1.3.0 release in PR #65 |
That's great news! I'm glad the performance optimization made it into the v1.3.0 release. |
1 participant
This change optimizes the database initialization process by batching the fetching of pragma settings into a single IPC call. Previously, 8 separate IPC calls were made to fetch each pragma individually.
The changes include:
natives/native-worker.js: Extracted the query execution logic into a reusableexecuteQueryfunction.queryBatchSupport: Implemented a newqueryBatchRPC method in the native worker that accepts an array of queries and returns an array of results.src/nativeWorker.ts: ModifiedgetPragmasto construct a batch of pragma queries and execute them usingqueryBatch.Performance benchmark showed a ~3.4x improvement in fetching pragmas.
PR created automatically by Jules for task 2295165269245791039 started by @zknpr