Skip to content

Conversation

@KaganCanSit
Copy link
Contributor

@KaganCanSit KaganCanSit commented Jul 15, 2025

This optimization was spotted by @reneme in review PR #4783. While there are more instances of `typecast_copy' in this codebase, I wanted to improve it with a small change for this part only.

Referenced review comment: #4783 (comment)

Quote:

General remark (please don't tackle in this pull request): This roughtime implementation makes heavy use of std::array<> in places where (nowadays) it would be better off to use statically-sized std::span.

Here, hashNode is a good example: if it would be implemented using spans, you could avoid the data copies and simplify the code:

   BufferSlicer slicer(path);
   for(size_t level = 0; level < levels; ++level) {
      hashNode(hash, slicer.take<64>(), index & 1);
      index >>= 1;
   }

There are likely other places in this implementation that could avoid the typecast_copy (which is just a fancy name for an unnecessary data-copy).

Following the merge of this branch, I will need to rebase #4783 against master. This should not present any issues. Please let me know if you require any modifications.

Best regards.

@KaganCanSit KaganCanSit changed the title Roughtime file hashNode function update with span Replace std::array with std::span in roughtime hashNode function Jul 15, 2025
Copy link
Owner

@randombit randombit left a comment

Choose a reason for hiding this comment

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

LGTM thanks

@KaganCanSit
Copy link
Contributor Author

LGTM thanks

Thanks for the quick response @randombit. I'm on Github today and will generally make time, but if you have time and see fit, I'd also love to complete and merge enhancement #4783 🤝.

Nice day.

@coveralls
Copy link

Coverage Status

coverage: 90.626% (+0.004%) from 90.622%
when pulling d9e1c67 on KaganCanSit:reduce-copy-for-rougtime-file-with-span
into e18675f on randombit:master.

@randombit randombit merged commit 70871ae into randombit:master Jul 15, 2025
45 checks passed
@KaganCanSit KaganCanSit deleted the reduce-copy-for-rougtime-file-with-span branch July 15, 2025 12:26
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