-
Notifications
You must be signed in to change notification settings - Fork 17
Replace compact_str with smol_str throughout the codebase #75
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
base: main
Are you sure you want to change the base?
Conversation
[APPROVALNOTIFIER] This PR is NOT APPROVED This pull-request has been approved by: The full list of commands accepted by this bot can be found here.
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |
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.
Summary of Changes
Hello @awgn, 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 refactors the project's string handling by systematically replacing the compact_str
crate with the smol_str
crate. The primary goal is to unify string management, leveraging smol_str
's superior performance characteristics, particularly in cloning and memory allocation, and its more ergonomic API. This change aims to enhance the overall efficiency and consistency of string operations throughout the application, contributing to a more robust and maintainable codebase.
Highlights
- String Type Standardization: Replaced
compact_str
withsmol_str
across the codebase to achieve more consistent and streamlined string handling, reducing cognitive load associated with multiple string libraries. - Performance and Efficiency Improvements: The switch to
smol_str
brings several advantages, including more efficient cloning (especially for strings longer than 23 characters) and construction, with cloning operations never involving dynamic memory allocation. This contributes to overall performance gains. - Codebase Quality and Maintainability:
smol_str
is noted for its higher quality and is used as a core library inrust-analyzer
, indicating a robust and well-maintained choice. This change improves the maintainability and reliability of the project's string operations. - Dependency Updates and Cleanup: Numerous
Cargo.toml
andCargo.lock
files were updated to reflect the removal ofcompact_str
and the addition/promotion ofsmol_str
as a workspace dependency. This also involved updating several other related dependencies and removing unused ones likecastaway
andoverload
. - Minor Behavioral Adjustments: Adjustments were made in
orion-lib/src/listeners/listener.rs
for comparingroute_id
withid
, changingroute_id == id
toroute_id == &id
. This reflects a subtle change in how string comparisons are handled with the newSmolStr
type.
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 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
-
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 systematically replaces compact_str
with smol_str
throughout the codebase to improve performance and streamline string handling. The changes are comprehensive, covering dependencies in Cargo.toml
and Cargo.lock
, as well as all usages in the source code. The migration has been handled carefully, correctly adapting to the API differences between the two crates. The rationale for this change is well-explained in the pull request description. The implementation is clean, and I have not found any issues. This is a solid improvement for the project.
However, not sure if the string length exceed 23, how much performance will be degragated |
There are several reasons for replacing |
Not sure the performance influcen to long string, it is very common the xds name exceed 23 characters |
Signed-off-by: Nicola Bonelli <[email protected]>
a178803
to
c0212da
Compare
Rationale
Currently, Orion utilizes a variety of string types, each chosen for specific performance or ergonomic reasons:
String
: Used for general-purpose, heap-allocated string data.CompactStr
: Mostly adopted for component names (e.g., listeners, clusters) due to its clone efficiency over the standardString
.SmolStr
: Used in the access logger as it proved to be more performant thanCompactStr
and offers a more ergonomic API.&'static str
(via a String Interner): Recently, we introduced a string interner to represent component names as&'static str
. This was a requirement for using them as long-lived tags in our OpenTelemetry integration (note: component's names have not been replaced but rather paired with their static str versions).To make our string handling more consistent and streamlined, and to reduce the cognitive load of dealing with a large number of string-handling libraries, this pull request replaces compact_str with the more modern and efficient smol_str.
Advantages of smol_str over compact_str: