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

perf: Remove unnecessary allocation and construction of a HashMap in Signature::parse #598

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mennovf
Copy link

@mennovf mennovf commented Aug 28, 2024

Summary

I noticed Signature::parse constructs a temporary HashMap for some reason. I removed this allocation and weird construction to speed up the parsing. I see no need for the wasteful temporary.

Checklist

@mzeitlin11
Copy link
Collaborator

Thanks for the pr, this looks good to me! Is pretty similar to the implementation in the next branch, but good to get in sooner.

Only semantic difference looks like missing = in signature chunk is ignored here, vs errored in next branch implementation.

@mennovf
Copy link
Author

mennovf commented Sep 1, 2024

I was unaware of the ongoing effort on the next branch when I made this pull request. I could copy over its code into this pull request or just close it and wait for next.

@arlyon
Copy link
Owner

arlyon commented Sep 3, 2024

I am ok with backporting it, running CI

@arlyon
Copy link
Owner

arlyon commented Sep 24, 2024

Hello, @mennovf, I have recently merged some changes to bump msrv and fix the CI issues. Please rebase when you have the chance :)

@mennovf mennovf force-pushed the signature_parse_remove_hashmap branch from bcbbe84 to de6d399 Compare September 30, 2024 21:00
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.

3 participants