Skip to content
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

feat: optimize HexString to use Cow<[u8]> for efficient memory usage #2675

Closed
wants to merge 3 commits into from

Conversation

wonderfan
Copy link

@wonderfan wonderfan commented Feb 6, 2025

Use this change set to fix #2657

Linked Issues/PRs

Description

Checklist

  • Breaking changes are clearly marked as such in the PR description and changelog
  • New behavior is reflected in tests
  • The specification matches the implemented behavior (link update PR if changes are needed)

Before requesting review

  • I have reviewed the code myself
  • I have created follow-up issues caused by this PR and linked them here

After merging, notify other teams

[Add or remove entries as needed]

@fuel-cla-bot
Copy link

fuel-cla-bot bot commented Feb 6, 2025

Thanks for the contribution! Before we can merge this, we need @wonderfan to sign the Fuel Labs Contributor License Agreement.

@wonderfan wonderfan marked this pull request as draft February 6, 2025 07:52
@rymnc
Copy link
Member

rymnc commented Feb 6, 2025

thanks for your PR @wonderfan :)

please let us know what you need from us

Copy link
Member

@rymnc rymnc left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

first pass, generally looks good, but it would be nice to remove any helpers which are unused

Ok(fuel_types::Nonce::from(bytes))
}
}

// Additional convenience methods
impl HexString {
pub fn as_bytes(&self) -> &[u8] {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn as_bytes(&self) -> &[u8] {
/// View the HexString as a byte array.
pub fn as_bytes(&self) -> &[u8] {

self.0.as_ref()
}

pub fn into_owned(self) -> Vec<u8> {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn into_owned(self) -> Vec<u8> {
/// Extracts the owned data.
/// Clones the data if it is not already owned.
pub fn into_owned(self) -> Vec<u8> {

self.0.into_owned()
}

pub fn from_static(bytes: &'static [u8]) -> Self {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
pub fn from_static(bytes: &'static [u8]) -> Self {
/// Convert a static byte array to a HexString.
pub fn from_static(bytes: &'static [u8]) -> Self {

@wonderfan wonderfan marked this pull request as ready for review February 7, 2025 06:50
@wonderfan
Copy link
Author

@rymnc I try to optimize the codes, but it introduces other side effects after tests. As a result, i close this PR.

@wonderfan wonderfan closed this Feb 7, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Avoid cloning of vectors in HexString
2 participants