Field: More efficient conversion into Bignum_bigint.t #14599
Conversation
0b05b93 to
709463d
Compare
|
!ci-build-me |
|
If I try these patches rebased on top of |
|
Just noticed that there is another implementation of that function in snarky with the same issue: |
efdf699 to
098f683
Compare
709463d to
6fcd939
Compare
| Bignum_bigint.hash_fold_t s (to_bignum_bigint (to_bigint x)) | ||
| (* For non-zero values, conversion is done by creating the bin_prot representation | ||
| of the [Bignum_bigit.t] value, and then parsing it with bin_prot. *) | ||
| match compare n zero with |
There was a problem hiding this comment.
Is this necessary? The bigint representation will always be positive, so it should be fine (and actually more efficient) to just special-case zero using an equality check.
There was a problem hiding this comment.
@mrmr1993 not at all, the only purpose of all this logic is to:
- Recover the sign because bin_prot requires it
- Avoid bypassing bignum when converting, but if that is not an issue I can use Zarith directly
There was a problem hiding this comment.
@mrmr1993 btw, are there any values that are common and are worth short-circuiting like zero? do you think it is worth adding one too ?
| Bytes.unsafe_set buf 1 size_byte ; | ||
| (* Copy the bytes representation of the value, skip the tag and size bytes *) | ||
| Bytes.unsafe_blit ~src:bytes ~src_pos:0 ~dst_pos:2 ~len ~dst:buf ; | ||
| Bin_prot.Reader.of_bytes Bignum_bigint.Stable.V1.bin_reader_t buf |
There was a problem hiding this comment.
Can you not use Z.of_bits directly? There should be a no-op converter to/from Bigint.t from there.
|
@mrmr1993 got rid of the intermediary binprot buffer construction and re-implemented the conversion using Zarith directly. Current version special-cases 0 and 1 so that the bytes representation is not created. |
|
!ci-build-me |
|
!ci-nightly-me |
|
!ci-nightly-me |
As discussed here, this is a more efficient conversion into
Bignum_bigint.tvalues that avoids processing the input and output bit by bit.It feels kind of dirty having to go through bin_prot for this, but
Bignum_bigintdoesn't expose any other way to interpret raw bytes to produce a bigint (which should be more efficient than the alternatives). A pro of this approach is that it didn't require any new function to be exposed (other than thecompareadded in the other field), but assumes that all implementations ofto_byteswill produce the same output.Explain how you tested your changes:
*
Checklist: