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

ballet: add base58 utils #75

Merged
merged 1 commit into from
Mar 6, 2023
Merged

ballet: add base58 utils #75

merged 1 commit into from
Mar 6, 2023

Conversation

ptaffet-jump
Copy link
Contributor

@ptaffet-jump ptaffet-jump commented Jan 27, 2023

Add high performance base58 conversion functions (encode and decode) for 32 and 64 bytes. The encoder functions have been especially optimized for AVX.

Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

Needs Bazel fix for CI builds

src/ballet/base58/BUILD Outdated Show resolved Hide resolved
@ripatel-fd
Copy link
Contributor

ripatel-fd commented Feb 2, 2023

@ptaffet-jump Looks like your test takes over 5 minutes to run with coverage enabled FYI.
Should be fine for now, but going forward, we could add a --test-short flag to tests to reduce the number of iterations.
And then run the tests twice in these two configurations:

  • Coverage disabled, but run long tests
  • Coverage enabled, only run short tests

What do you think?

@ripatel-fd
Copy link
Contributor

@ptaffet-jump CI with code coverage actually timed out after 600s. Would you mind adding that --test-short flag to reduce iterations? I'll do the Bazel side of this integration

@ptaffet-jump
Copy link
Contributor Author

ptaffet-jump commented Feb 6, 2023

If you now run build/linux/gcc/x86_64/unit-test/test_base58 --cnt 1000 it finishes in a couple of seconds. The default still does 10,000,000 iterations. I don't know how to change CI to add the flag.

@ptaffet-jump ptaffet-jump marked this pull request as ready for review February 6, 2023 23:14
src/ballet/base58/BUILD Outdated Show resolved Hide resolved
Copy link
Contributor

@ripatel-fd ripatel-fd left a comment

Choose a reason for hiding this comment

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

Dismissing my "request changes" review to "neutral".

@codecov
Copy link

codecov bot commented Feb 10, 2023

Codecov Report

Patch coverage: 100.00% and project coverage change: +36.68 🎉

Comparison is base (3600cdc) 55.13% compared to head (9dd08f0) 91.81%.

❗ Current head 9dd08f0 differs from pull request most recent head 5faf8c4. Consider uploading reports for the commit 5faf8c4 to get more accurate results

Additional details and impacted files
@@             Coverage Diff             @@
##             main      #75       +/-   ##
===========================================
+ Coverage   55.13%   91.81%   +36.68%     
===========================================
  Files         179      102       -77     
  Lines       22707     7942    -14765     
  Branches     5733        0     -5733     
===========================================
- Hits        12519     7292     -5227     
+ Misses       6501      650     -5851     
+ Partials     3687        0     -3687     
Impacted Files Coverage Δ
src/ballet/base58/test_base58.c 100.00% <100.00%> (ø)
src/util/scratch/fd_scratch.h 0.00% <0.00%> (-91.90%) ⬇️
src/util/cstr/fd_cstr.h 0.00% <0.00%> (-79.11%) ⬇️
src/util/math/fd_stat.h 20.00% <0.00%> (-63.34%) ⬇️
src/util/pod/fd_pod.h 20.83% <0.00%> (-62.50%) ⬇️
src/tango/fctl/fd_fctl.h 20.00% <0.00%> (-53.75%) ⬇️
src/util/shmem/fd_shmem.h 0.00% <0.00%> (-6.67%) ⬇️
src/util/fd_hash.c 100.00% <0.00%> (ø)
src/util/fd_util.c 100.00% <0.00%> (ø)
src/util/test_util.c 100.00% <0.00%> (ø)
... and 176 more

Help us with your feedback. Take ten seconds to tell us how you rate us. Have a feature suggestion? Share it here.

☔ View full report at Codecov.
📢 Do you have feedback about the report comment? Let us know in this issue.

@ptaffet-jump ptaffet-jump force-pushed the ptaffet/base58 branch 2 times, most recently from 9dd08f0 to 8f80b0c Compare February 13, 2023 22:52
@ptaffet-jump ptaffet-jump force-pushed the ptaffet/base58 branch 2 times, most recently from fb974af to c424ab4 Compare February 15, 2023 15:42
@kbowers-jump kbowers-jump added this pull request to the merge queue Mar 6, 2023
Merged via the queue into main with commit 3fdabed Mar 6, 2023
@kbowers-jump kbowers-jump deleted the ptaffet/base58 branch March 6, 2023 21:11
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