Const driven size reductions #29
Conversation
Reviewer's GuideReplaces the 10% division-based buffer escape_reserve computation with a 12.5% bit-shift operation to avoid 32-bit division overhead on 8-bit AVR platforms, and updates the corresponding test expectation. File-Level Changes
Tips and commandsInteracting with Sourcery
Customizing Your ExperienceAccess your dashboard to:
Getting Help
|
WalkthroughThe calculation for the Changes
Poem
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 Clippy (1.86.0)warning: failed to write cache, path: /usr/local/registry/index/index.crates.io-1949cf8c6b5b557f/.cache/de/fm/defmt, error: Permission denied (os error 13) Caused by: ✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Summary of Changes
Hello @kaidokert, 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 introduces a targeted optimization within the DirectBuffer struct by modifying how the escape_reserve is calculated. The change replaces a general integer division with a bitwise right shift, which is a common technique to improve performance and reduce compiled code size, particularly for resource-constrained embedded systems like 8-bit AVR microcontrollers. The PR also ensures that documentation and tests are updated to reflect this revised calculation.
Highlights
- Performance Optimization: Replaced an integer division (
/ 10) with a bitwise right shift (>> 3) in theDirectBuffer'sescape_reservecalculation. This change is specifically aimed at optimizing performance and potentially reducing code size on 8-bit AVR targets by avoiding expensive 32-bit division operations. - Logic Adjustment & Clarity: The
escape_reservecalculation now reserves approximately 12.5% of the buffer (instead of 10%), and the corresponding inline comment and unit test assertion have been updated to reflect this new logic and the rationale behind the change.
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.
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.
Code Review
This pull request optimizes the calculation of escape_reserve in DirectBuffer by replacing a division with a bit-shift operation, improving performance on resource-constrained devices. The change increases the reserved space, potentially reducing available space for unescaped strings.
![sourcery-ai[bot]](https://avatars.githubusercontent.com/in/48477?s=60&v=4){kind=small}
There was a problem hiding this comment.
Hey @kaidokert - I've reviewed your changes - here's some feedback:
- Consider making the shift-based reserve calculation conditional with cfg(target_arch = "avr") so that non-AVR targets still use the exact 10% division semantics.
- Add tests for buffer lengths around the 64-byte threshold (e.g., just below and above 512) to verify the .max(64) and >>3 behavior at edge cases.
Prompt for AI Agents
Please address the comments from this code review:
## Overall Comments
- Consider making the shift-based reserve calculation conditional with cfg(target_arch = "avr") so that non-AVR targets still use the exact 10% division semantics.
- Add tests for buffer lengths around the 64-byte threshold (e.g., just below and above 512) to verify the .max(64) and >>3 behavior at edge cases.Help me be more useful! Please click 👍 or 👎 on each comment and I'll use the feedback to improve your reviews.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (3)
picojson/src/direct_buffer.rs (3)
228-228: Update outdated comment for consistency.The comment still references the old 10% calculation. Consider updating it to reflect the new ~12.5% calculation.
- assert_eq!(db.escape_reserve, 64); // 10% of 100, minimum 64 + assert_eq!(db.escape_reserve, 64); // ~12.5% of 100, minimum 64
342-342: Update outdated comment for consistency.The comment still references the old calculation method. Consider updating it to reflect the new bit-shift approach.
- assert_eq!(db.escape_reserve, 64); // max(100/10, 64) = 64 + assert_eq!(db.escape_reserve, 64); // max(100>>3, 64) = 64
418-418: Update outdated comment for consistency.The comment still references the old 10% calculation. Consider updating it to reflect the new ~12.5% calculation.
- // Check escape reserve was set correctly (10% of 100, minimum 64) + // Check escape reserve was set correctly (~12.5% of 100, minimum 64)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
picojson/src/direct_buffer.rs(2 hunks)
🧰 Additional context used
🧠 Learnings (2)
📓 Common learnings
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#18
File: avr_demo/src/stack_measurement.rs:39-39
Timestamp: 2025-07-02T06:17:39.995Z
Learning: In AVR stack measurement implementations for ATmega2560, when the stack grows downward from RAMEND and watermark scanning finds the first overwritten byte at current_ptr, the calculation `RAMEND_ADDR - current_ptr` correctly reports actual stack usage (bytes consumed), not remaining bytes. This pattern is used in the picojson-rs avr_demo stack measurement module.
picojson/src/direct_buffer.rs (3)
Learnt from: kaidokert
PR: kaidokert/picojson-rs#1
File: tokenizer/src/bitstack/mod.rs:0-0
Timestamp: 2025-06-28T23:43:22.754Z
Learning: In the picojson-rs project, the BitStack trait was redesigned to return bool instead of Option<bool> for pop() and top() methods. Empty stacks return false rather than None, which simplifies the API and avoids Option handling.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#18
File: avr_demo/src/stack_measurement.rs:39-39
Timestamp: 2025-07-02T06:17:39.995Z
Learning: In AVR stack measurement implementations for ATmega2560, when the stack grows downward from RAMEND and watermark scanning finds the first overwritten byte at current_ptr, the calculation `RAMEND_ADDR - current_ptr` correctly reports actual stack usage (bytes consumed), not remaining bytes. This pattern is used in the picojson-rs avr_demo stack measurement module.
Learnt from: kaidokert
PR: kaidokert/picojson-rs#5
File: picojson/src/lib.rs:0-0
Timestamp: 2025-06-29T17:48:18.188Z
Learning: In the picojson-rs project, the `use tokenizer as ujson;` alias in lib.rs is a transitionary and fully internal private alias used during crate reorganization. Examples and external code no longer depend on this alias, making the private visibility appropriate.
🔇 Additional comments (2)
picojson/src/direct_buffer.rs (2)
49-51: Excellent performance optimization for AVR targets!The change from division to bit-shift operation is a smart optimization that avoids expensive 32-bit division on 8-bit AVR targets. The math is correct (
>> 3=/8= 12.5%), and the updated comments clearly explain both the change and the motivation.
352-352: Test assertion correctly updated.The test assertion properly reflects the new calculation:
1000 >> 3 = 125instead of the previous1000 / 10 = 100.
Summary by Sourcery
Increase the escape buffer reservation to ~12.5% by replacing division with a bit-shift for performance on AVR targets and update the corresponding unit test
Enhancements:
Tests:
Summary by CodeRabbit
Refactor
Tests