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: Implement fixed length hash functions and optimize existing imlementations #646

Merged
merged 7 commits into from
Dec 16, 2021

Conversation

austinabell
Copy link
Contributor

@austinabell austinabell commented Nov 25, 2021

  • Adds fixed length array versions of env hash functions (no reason to allocate if we know the length)
  • Optimizes the existing impl to use this to avoid an extra syscall to register_len
    • I am assuming the compiler can optimize the creation on stack and copy to heap, but if not the cost should be trivial and these functions will probably be deprecated in future
  • Added helper functions for reading to fixed-length arrays for re-use

I don't necessarily like suffixing _hash, but ideally this is a smooth transition to avoid hard-breaking changes if users expect a Vec<u8> over the array, even though this comes with a major version bump. Name suggestions welcome!

closes #645

@ChaoticTempest
Copy link
Member

how about something like _unboxed instead of _hash? I think at least that would convey the difference, and even if we deprecate the one without the postfix, I think it still makes sense

@austinabell
Copy link
Contributor Author

how about something like _unboxed instead of _hash? I think at least that would convey the difference, and even if we deprecate the one without the postfix, I think it still makes sense

I personally am confused with this wording, because I don't see what boxed refers to here or what unboxed means. What do you mean by this?

@ChaoticTempest
Copy link
Member

@austinabell ahh my bad, I meant to say that _unboxed would show that you're returning something that isn't boxed/allocated inside something like a Vec<u8>, which would show the difference between the two functions. If you're confused by it, then it probably isn't good naming either then

@austinabell
Copy link
Contributor Author

@austinabell ahh my bad, I meant to say that _unboxed would show that you're returning something that isn't boxed/allocated inside something like a Vec<u8>, which would show the difference between the two functions. If you're confused by it, then it probably isn't good naming either then

Could also suffix something like _array, to be descriptive, but I just thought that _hash would be most clear because at least it's explaining that these are hashes which might not be intuitive for those unfamiliar.

@ChaoticTempest
Copy link
Member

Could also suffix something like _array, to be descriptive, but I just thought that _hash would be most clear because at least it's explaining that these are hashes which might not be intuitive for those unfamiliar.

But aren't they both hashes though? I think _array works since it should distinguish what's the different between the two return types

@austinabell
Copy link
Contributor Author

Could also suffix something like _array, to be descriptive, but I just thought that _hash would be most clear because at least it's explaining that these are hashes which might not be intuitive for those unfamiliar.

But aren't they both hashes though? I think _array works since it should distinguish what's the different between the two return types

Was just giving a benefit to using hash suffix. I'll switch to array suffix since it might be more clear :)

Copy link
Member

@ChaoticTempest ChaoticTempest left a comment

Choose a reason for hiding this comment

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

New changes LGTM

@austinabell austinabell merged commit e78bb70 into master Dec 16, 2021
@austinabell austinabell deleted the austin/fixed_hashes branch December 16, 2021 16:32
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.

Switch API for env hash functions to return array
2 participants