-
Notifications
You must be signed in to change notification settings - Fork 86
Use a single TextEncoder instance
#274
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Use a single TextEncoder instance
#274
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hello @grantcox, 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!
Summary of Changes
This pull request introduces a performance optimization by using a single TextEncoder instance instead of creating a new one for each string encoding. It also adds a new benchmark to measure the impact of this change. The changes affect the sqlite-api.js file by introducing a textEncoder variable and using it in the createUTF8 function. Additionally, the benchmark suite is expanded to include a test case with 100K SELECT statements with bound string parameters, which required changes to benchmarks.html, benchmarks.js, and demo-worker.js.
Highlights
- Performance Optimization: The primary goal is to improve performance by reusing a single
TextEncoderinstance, reducing the overhead of creating new instances for each string encoding. - New Benchmark: A new benchmark (Test 17) is added to specifically measure the performance impact of the
TextEncoderoptimization when using bound string parameters. - Parameter Binding: The
demo-worker.jsfile is modified to handle parameterized queries, allowing the new benchmark to execute SELECT statements with bound string parameters.
Changelog
Click here to see the changelog
- demo/benchmarks/benchmarks.html
- Added a new table row for 'Test 17: 100K SELECTs with bound string parameters' at line 107.
- demo/benchmarks/benchmarks.js
- Added a function
benchmark100kSelectsWithParamsto generate an array of 100,000 SELECT statements with parameters, starting at line 5. - Modified
benchmarksReadyto include the new benchmark with bound parameters, starting at line 15. - Renamed
sqlparameter toqueryin thequeryfunction at line 110.
- Added a function
- demo/demo-worker.js
- Modified the message handler to process both single SQL strings and arrays of queries (SQL + params), starting at line 135.
- Added logic to bind parameters to the prepared statement using
sqlite3.bind_collectionat line 150.
- src/sqlite-api.js
- Introduced a
textEncoderconstant at line 37 to hold a single instance ofTextEncoder. - Replaced
new TextEncoder()withtextEncoderin thecreateUTF8function at line 42. - Replaced
new TextEncoder()withtextEncoderwhen encoding the SQL string to UTF-8 at line 663.
- Introduced a
Using Gemini Code Assist
The 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 in your pull request via creating an issue comment (i.e. comment on the pull request page) using either /gemini <command> or @gemini-code-assist <command>. Below is a summary of the supported commands.
| Feature | Command | Description |
|---|---|---|
| Code Review | /gemini review |
Performs a code review for the current pull request in its current state. |
| Pull Request Summary | /gemini summary |
Provides a summary of the current pull request in its current state. |
| Comment | @gemini-code-assist | Responds in comments when explicitly tagged, both in issue comments and review comments. |
| Help | /gemini help |
Displays a list of available commands. |
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 .gemini/ folder in the base of the repository. Detailed instructions can be found here.
Limitations & Feedback
Gemini Code Assist is currently in preview and 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 to provide feedback.
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.
A coder's quest for speed,
TextEncoder's wasteful deed,
One instance to bind,
A faster find,
Efficiency's planted seed.
Footnotes
-
Review the Privacy Notices, Generative AI Prohibited Use Policy, Terms of Service, and learn how to configure Gemini Code Assist in GitHub here. Gemini can make mistakes, so double check it and use code with caution. ↩
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Code Review
This pull request introduces a performance improvement by using a single TextEncoder instance instead of creating a new one for each string encoding. The change is well-explained and includes a benchmark demonstrating the impact. The code changes look good overall.
Summary of Findings
- Missing comments: Consider adding comments to explain the purpose of the string arguments in
benchmark100kSelectsWithParamsand the order in which benchmarks are loaded. - Missing comment: Consider adding a comment to explain the performance benefits of initializing
TextEncoderonce.
Merge Readiness
The pull request is well-structured and addresses the stated goal of improving performance. The changes are relatively small and the inclusion of a benchmark is commendable. I recommend merging after addressing the comments above. I am unable to directly approve this pull request, so please have others review and approve this code before merging.
| const tmp = Module._malloc(8); | ||
| const tmpPtr = [tmp, tmp + 4]; | ||
|
|
||
| const textEncoder = new TextEncoder(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is a good optimization! Initializing TextEncoder once and reusing it avoids unnecessary object creation. Consider adding a comment explaining why this is being done, referencing the performance benefits.
// Initialize TextEncoder once for performance to avoid repeated object creation
const textEncoder = new TextEncoder();|
Thanks for the PR! I'm fine with the actual code change, but I don't want to add the extra benchmark as that set of benchmarks is based on another pre-existing set. |
b626d57 to
848ca54
Compare
|
No problem, I've dropped that commit now. |
This is a minor performance improvement. Currently a new
TextEncoderinstance is created for every JS string that is encoded into a byte array. Creating new instances every time is unnecessary AFAICT, and a singleTextEncodercan be used for all conversions.I've added a new benchmark to demonstrate the impact of this (see the bottom table row) - it's fairly minor, but measurable in our workflow.
Checklist
non-exclusive, royalty-free, irrevocable copyright license to reproduce, prepare
derivative works of, publicly display, sublicense, and distribute this
Contribution and such derivative works.
Contribution contains no content requiring a license from any third party.