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

Updated dependencies #4

Merged
merged 5 commits into from
Jun 19, 2024
Merged

Updated dependencies #4

merged 5 commits into from
Jun 19, 2024

Conversation

parrottq
Copy link
Contributor

I was thinking of using this dependency for a project but saw that it was pretty out of date so I spent a little bit of time cleaning it up.

Changes

  • Migrated to latest edition
  • Updated all the dependencies
  • Removed lazy_static because it is no longer needed with new versions of base64
  • Fixed the feature flags to be opt-in

Testing

All the units tests except the ones related to diesel pass. I'm getting a link error when I enable the diesel feature flag but I assume that they still pass if you compile the binary in the right environment.

Improved Codegen

I took a look at the asm generated for this snippet on both the current version (master) and after these changes to see the difference in codegen:

// With `lto = true`
#[no_mangle]
#[inline(never)]
pub fn convert(e: UuidB64) -> InlineString {
    e.to_istring()
}

On master there are lots of branch and allocation calls; meanwhile after updating base64 the above function now only contains the following instructions on x86:
add, lea, mov, movabs, movups, movzx, or, ret, shr, sub

I haven't measured the performance difference with a benchmark but presumably this new version will be much faster.

Copy link
Owner

@quodlibetor quodlibetor left a comment

Choose a reason for hiding this comment

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

Thanks for this, looks great!

static ref B64_CONFIG: Config = Config::new(
CharacterSet::UrlSafe,
false, // pad?
false, // trim whitespace?
Copy link
Owner

Choose a reason for hiding this comment

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

I guess the base64 crate no longer handles whitespace itself (marshallpierce/rust-base64#163) which seems legitimate.

@quodlibetor
Copy link
Owner

I'm updating the CI in this repo since I haven't touched it really since Travis CI went kapoot, I'll merge this in the next couple days after I get that working.

Thanks again!

@quodlibetor quodlibetor merged commit 1156984 into quodlibetor:master Jun 19, 2024
3 checks passed
@quodlibetor
Copy link
Owner

0.2.0 has been released, thanks for this!

@parrottq parrottq deleted the modernize branch June 20, 2024 12:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants