Skip to content
This repository has been archived by the owner on Nov 15, 2023. It is now read-only.

chore: reduce copy times for bytes in core-hashing #13519

Merged

Conversation

yjhmelody
Copy link
Contributor

@yjhmelody yjhmelody commented Mar 3, 2023

  • I do not konw if these costs(copy_from_slice) will be erased by rust compiler, so I improve the hash code by hand.
  • Reduce duplicated code

@yjhmelody yjhmelody changed the title chore: reduce copy bytes for core-hashing chore: reduce copy times for bytes in core-hashing Mar 3, 2023
primitives/core/hashing/src/lib.rs Outdated Show resolved Hide resolved
/// Do a Blake2 64-bit hash and place result in `dest`.
pub fn blake2_64_into(data: &[u8], dest: &mut [u8; 8]) {
type Blake2b64 = blake2::Blake2b<U8>;
Copy link
Member

Choose a reason for hiding this comment

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

Are the *_into(data, dest) family of functions relevant?
Looks like are not used anywere in the codebase and is some user requires something like that it can easily construct it

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Do you means that should I remove these xx_into functions?

Copy link
Member

Choose a reason for hiding this comment

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

I usually tend to follow YAGNI principle.
But with that I'm not saying that should be a dogma.
I have the (maybe bad?!) attitude to remove unused code especially the trivial bloat that can be trivially implemented using a line of code by the user 😃

@davxy davxy added B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit A0-pleasereview labels Mar 3, 2023
@davxy davxy requested a review from a team March 3, 2023 14:28
@koute
Copy link
Contributor

koute commented Mar 3, 2023

  • I do not konw if these costs(copy_from_slice) will be erased by rust compiler, so I improve the hash code by hand.

Besides checking the assembly there's also a benchmark in primitives/core/benches/bench.rs for blake2_128; you could run it before and after and see if there's a difference.

@davxy
Copy link
Member

davxy commented Mar 3, 2023

  • I do not konw if these costs(copy_from_slice) will be erased by rust compiler, so I improve the hash code by hand.

Besides checking the assembly there's also a benchmark in primitives/core/benches/bench.rs for blake2_128; you could run it before and after and see if there's a difference.

IMO copying 32 or 64 bytes in a modern machine is so fast that maybe the difference is hard to be appreciated.
Especially if the operation is performed in the context of a hash where internally copies the data one order of magnitude more.
(But I haven't tried... so I'm just guessing)

Nevertheless I'm almost always ok to code pruning if there are no functional changes

Copy link
Contributor

@melekes melekes left a comment

Choose a reason for hiding this comment

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

👍

@koute
Copy link
Contributor

koute commented Mar 6, 2023

Nevertheless I'm almost always ok to code pruning if there are no functional changes

Yeah, I agree, I was mostly just suggesting that as a curiosity as nevertheless it'd be nice to know.

@koute
Copy link
Contributor

koute commented Mar 6, 2023

bot merge

@paritytech-processbot paritytech-processbot bot merged commit 435446f into paritytech:master Mar 6, 2023
@koute
Copy link
Contributor

koute commented Mar 6, 2023

@yjhmelody Thanks!

@yjhmelody yjhmelody deleted the chore-improve-core-hashing branch March 7, 2023 02:15
ukint-vs pushed a commit to gear-tech/substrate that referenced this pull request Apr 10, 2023
* chore: reduce copy bytes for core-hashing

* improve by suggestions and remove unused `xx_into`

* chore: replace sha2 crate by `sp_core::hashing` for pallet-alliance

* fix features

* use sp-core-hashing directly

* add to dev-dep
nathanwhit pushed a commit to nathanwhit/substrate that referenced this pull request Jul 19, 2023
* chore: reduce copy bytes for core-hashing

* improve by suggestions and remove unused `xx_into`

* chore: replace sha2 crate by `sp_core::hashing` for pallet-alliance

* fix features

* use sp-core-hashing directly

* add to dev-dep
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
B0-silent Changes should not be mentioned in any release notes C1-low PR touches the given topic and has a low impact on builders. D3-trivial 🧸 PR contains trivial changes in a runtime directory that do not require an audit
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants