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

Chore: Replace Hex Encoding/Decoding #4465

Draft
wants to merge 3 commits into
base: next
Choose a base branch
from

Conversation

ASuciuX
Copy link
Contributor

@ASuciuX ASuciuX commented Mar 1, 2024

Description

I took a look at the functions that were doing the hex encoding and decoding in the stacks-common package and there was an easy replacement with an external crate (faster-hex) in order to improve the performance. The results show a performance increase of about 1% when processing blocks:

$ hyperfine -w 3 -r 10 "./stacks-inspect-hex-modifications replay-block ~/chainstate/ range 99990 100000" "./stacks-inspect-9385b4236 replay-block ~/chainstate/ range 99990 100000"
Benchmark 1: ./stacks-inspect-hex-modifications replay-block ~/chainstate/ range 99990 100000
  Time (mean ± σ):      7.956 s ±  0.036 s    [User: 7.571 s, System: 0.352 s]
  Range (min … max):    7.929 s …  8.052 s    10 runs
 
Benchmark 2: ./stacks-inspect-9385b4236 replay-block ~/chainstate/ range 99990 100000
  Time (mean ± σ):      8.023 s ±  0.021 s    [User: 7.610 s, System: 0.379 s]
  Range (min … max):    7.992 s …  8.061 s    10 runs
 
Summary
  ./stacks-inspect-hex-modifications replay-block ~/chainstate/ range 99990 100000 ran
    1.01 ± 0.01 times faster than ./stacks-inspect-9385b4236 replay-block ~/chainstate/ range 99990 100000

@ASuciuX ASuciuX requested review from jcnelson and kantai March 1, 2024 13:26
@ASuciuX
Copy link
Contributor Author

ASuciuX commented Mar 1, 2024

@jcnelson @kantai From the benchmarking results, the implementation with faster-hex is better, but also it adds an extra dependency. Please let me know if you think this is worth adding.

jbencin
jbencin previously approved these changes Mar 1, 2024
Copy link
Contributor

@jbencin jbencin left a comment

Choose a reason for hiding this comment

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

Looks good. Smaller improvement than I expected. It's up to Aaron and Jude if they are okay introducing a dependency for this

@zone117x
Copy link
Member

zone117x commented Mar 4, 2024

Might also consider the hex-simd package which is ~2-3x faster than faster-hex https://nugine.github.io/simd-benches/

Copy link

codecov bot commented Mar 4, 2024

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 78.19%. Comparing base (383d586) to head (3317695).
Report is 19 commits behind head on next.

Additional details and impacted files
@@            Coverage Diff             @@
##             next    #4465      +/-   ##
==========================================
- Coverage   83.46%   78.19%   -5.28%     
==========================================
  Files         448      448              
  Lines      324321   324311      -10     
==========================================
- Hits       270698   253585   -17113     
- Misses      53623    70726   +17103     
Files Coverage Δ
stacks-common/src/util/hash.rs 80.60% <100.00%> (-0.57%) ⬇️
stacks-common/src/util/macros.rs 83.98% <100.00%> (+0.12%) ⬆️
stacks-common/src/util/mod.rs 44.82% <ø> (-8.63%) ⬇️
stackslib/src/blockstack_cli.rs 77.37% <100.00%> (-2.79%) ⬇️

... and 165 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 383d586...3317695. Read the comment docs.

@jcnelson
Copy link
Member

jcnelson commented Mar 4, 2024

Per the call today, I think we should defer further consideration of this until we remove hex encode/decode from the DB I/O paths (since that's where a lot of this happens, and in this case, we can just remove the hex codec altogether).

@ASuciuX
Copy link
Contributor Author

ASuciuX commented Mar 5, 2024

Per the call today, I think we should defer further consideration of this until we remove hex encode/decode from the DB I/O paths (since that's where a lot of this happens, and in this case, we can just remove the hex codec altogether).

I'll convert this into draft till then.

@ASuciuX ASuciuX marked this pull request as draft March 5, 2024 14:51
@kantai kantai removed their request for review May 2, 2024 14:14
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.

4 participants